[fix]: mask param in page.screenshot() only applied to first node#1612
Merged
seanmcguire12 merged 6 commits intomainfrom Jan 27, 2026
Merged
[fix]: mask param in page.screenshot() only applied to first node#1612seanmcguire12 merged 6 commits intomainfrom
mask param in page.screenshot() only applied to first node#1612seanmcguire12 merged 6 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: 7470fb6 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 SummaryThis PR fixes issue #1585 where the Key changes:
The implementation correctly addresses the previous review concern about Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Page
participant ScreenshotUtils
participant Locator
participant SelectorResolver
participant CDP
User->>Page: screenshot({mask: [locator]})
Page->>ScreenshotUtils: applyMaskOverlays(locators)
loop for each mask locator
ScreenshotUtils->>ScreenshotUtils: resolveMaskRects(locator)
ScreenshotUtils->>Locator: resolveNodesForMask()
alt nthIndex >= 0 (explicit nth())
Locator->>SelectorResolver: resolveAtIndex(query, nthIndex)
SelectorResolver->>CDP: evaluate element at index
CDP-->>SelectorResolver: {objectId, nodeId}
SelectorResolver-->>Locator: [single resolved node]
else nthIndex < 0 (all elements)
Locator->>SelectorResolver: resolveAll(query)
SelectorResolver->>CDP: evaluate all matching elements
CDP-->>SelectorResolver: [{objectId, nodeId}, ...]
SelectorResolver-->>Locator: [all resolved nodes]
end
Locator-->>ScreenshotUtils: resolved nodes with objectIds
loop for each resolved node
ScreenshotUtils->>ScreenshotUtils: resolveMaskRectForObject(objectId)
ScreenshotUtils->>CDP: callFunctionOn(getBoundingClientRect)
CDP-->>ScreenshotUtils: {x, y, width, height}
ScreenshotUtils->>CDP: releaseObject(objectId)
Note right of ScreenshotUtils: Always releases objectId<br/>even on failure
end
ScreenshotUtils-->>ScreenshotUtils: {frame, rects: [rect1, rect2, ...]}
end
ScreenshotUtils->>Page: inject mask overlay for all rects
Page->>CDP: screenshot with overlays
CDP-->>User: screenshot buffer
|
![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.
2 issues found across 4 files
Confidence score: 3/5
- Some risk due to a user-facing selector behavior:
.nth(0)inpackages/core/lib/v3/understudy/locator.tswon’t narrow results becausethis.nthIndex > 0excludes zero, which can change element targeting. - Potential resource leak in
packages/core/lib/v3/understudy/screenshotUtils.tsifresolveMaskRectForObjectthrows mid-iteration, leaving unreleasedobjectIds and possibly degrading stability. - Overall impact looks moderate (mid-severity issues, no evidence of tests or guards here), so this isn’t a blocker but warrants attention before merge.
- Pay close attention to
packages/core/lib/v3/understudy/locator.tsandpackages/core/lib/v3/understudy/screenshotUtils.ts- selector narrowing correctness and object release on failure.
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/understudy/locator.ts">
<violation number="1" location="packages/core/lib/v3/understudy/locator.ts:875">
P2: The condition `this.nthIndex > 0` doesn't handle `.nth(0)` as expected. Since `nthIndex` defaults to `0`, calling `.nth(0)` won't narrow to just the first element - it will return all elements instead. The JSDoc comment is misleading because `.nth(0)` doesn't actually narrow the result.
Consider either:
1. Tracking whether `.nth()` was explicitly called (e.g., with a separate flag or using `undefined`/`null` as the default)
2. Update the comment to clarify that `.nth(0)` behaves the same as no `.nth()` call</violation>
</file>
<file name="packages/core/lib/v3/understudy/screenshotUtils.ts">
<violation number="1" location="packages/core/lib/v3/understudy/screenshotUtils.ts:300">
P2: Potential resource leak: if `resolveMaskRectForObject` throws mid-iteration, remaining `objectId`s in `resolved` won't be released. Consider releasing all objectIds in an outer finally block that covers the entire iteration.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Process as applyMaskOverlays
participant Utils as resolveMaskRects (Internal)
participant Loc as Locator
participant CDP as CDPSession (Browser)
Note over Process,CDP: Logic flow for calculating mask coordinates
loop For each Locator in mask options
Process->>Utils: CHANGED: resolveMaskRects(locator)
Note over Utils,Loc: NEW: Resolves list of nodes instead of single node
Utils->>Loc: NEW: resolveNodesForMask()
alt Locator uses .nth(i)
Loc->>CDP: Resolve single node at index
CDP-->>Loc: Single ObjectId
else Default (All matches)
Loc->>CDP: CHANGED: Resolve ALL matching nodes
CDP-->>Loc: List of ObjectIds
end
Loc-->>Utils: Return Array<{ objectId }>
loop NEW: Iterate through every resolved node
Utils->>Utils: resolveMaskRectForObject(objectId)
Utils->>CDP: Runtime.callFunctionOn (getBoundingClientRect)
alt Element is visible & has dimensions
CDP-->>Utils: { x, y, width, height }
else Hidden or empty
CDP-->>Utils: null
end
Utils->>CDP: Runtime.releaseObject
end
Utils-->>Process: Return aggregated list of Rects
Process->>Process: Add Rects to frame overlay list
end
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Member
Author
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.
1 issue found across 5 files
Confidence score: 4/5
- This PR looks safe to merge; the only noted issue is a minor, non-functional error-message inconsistency.
packages/core/lib/v3/understudy/frameLocator.tshas a misleading error message about.nth()which could confuse users but doesn't affect runtime behavior.- Pay close attention to
packages/core/lib/v3/understudy/frameLocator.ts- misleading.nth()error message.
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/understudy/frameLocator.ts">
<violation number="1" location="packages/core/lib/v3/understudy/frameLocator.ts:158">
P2: Error message is misleading. `FrameLocator` doesn't have a `.nth()` method—this method is on `LocatorDelegate` which is returned by `frameLocator().locator()`. For consistency with the `Locator` class, the message should reference the locator context, e.g., `"locator().nth() expects a non-negative index"`.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Client
participant Utils as screenshotUtils
participant Loc as Locator
participant Res as SelectorResolver
participant CDP as CDPSession (Browser)
Note over Client,CDP: NEW: Multi-element Masking Flow
Client->>Utils: page.screenshot({ mask: locator })
Utils->>Loc: NEW: resolveNodesForMask()
alt Locator has explicit .nth(i)
Loc->>Res: resolveAtIndex(i)
Res-->>Loc: [Single Node]
else NEW: Default (Mask all matches)
Loc->>Res: resolveAll()
Res-->>Loc: [Multiple Nodes]
end
Loc-->>Utils: List of RemoteObjectIds
loop NEW: For each ObjectId
Utils->>CDP: Runtime.callFunctionOn(getBoundingClientRect)
alt Element visible & valid
CDP-->>Utils: Rect {x, y, w, h}
else Hidden or Invalid
CDP-->>Utils: null
end
Utils->>CDP: Runtime.releaseObject
end
Utils->>Utils: Aggregate valid rects
Utils-->>Client: Screenshot with N masks
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.
No issues found across 5 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Architecture diagram
sequenceDiagram
participant User as Test/Client
participant Utils as ScreenshotUtils
participant Loc as Locator
participant CDP as Chrome Session
User->>Utils: page.screenshot({ mask: [locator] })
Utils->>Utils: applyMaskOverlays()
Note over Utils,CDP: CHANGED: Logic now handles multiple nodes per locator
Utils->>Loc: NEW: resolveNodesForMask()
alt Locator has explicit .nth(index)
Loc->>CDP: Resolve single node at index
CDP-->>Loc: Returns single objectId
else NEW: Default behavior (no .nth)
Loc->>CDP: Resolve ALL matching nodes
CDP-->>Loc: Returns list of objectIds
end
Loc-->>Utils: Array<{ objectId, nodeId }>
loop NEW: Iterate over all resolved objects
Utils->>CDP: Runtime.callFunctionOn(getBoundingClientRect)
CDP-->>Utils: Rect {x, y, width, height}
Utils->>CDP: Runtime.releaseObject(objectId)
Utils->>Utils: Collect valid rect
end
Utils->>Utils: Draw masks over all collected rects
Utils-->>User: Return Screenshot Buffer
miguelg719
approved these changes
Jan 27, 2026
This was referenced Jan 27, 2026
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
2 participants
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
maskparam should be applied to all nodes that the locator resolves to (unless.nth()is specified)what changed
resolveMaskRect()toresolveMaskRects(), and updated behaviour to resolve all bounding rects instead of just 1locator.resolveNodesForMask()which is called byresolveMaskRects()and resolves all nodes & object IDs so they can be iterated through, and have masks applied to themtest plan
page-screenshot.spec.tsto cover the case where multiple elements match themasklocator