Repository navigation
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 07e460f461
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return; | ||
| } | ||
|
|
||
| const pixelRatio = imageSize.width / page.viewportSizeInCss.width; |
There was a problem hiding this comment.
When pixel-ratio validation runs with screenshotMode: "fullpage" (or auto detects a full-page bitmap), capturedImage.uncroppedSize contains the entire document because Camera.captureViewportImage records it before cropping. Dividing that width by the CSS viewport width therefore interprets document extent as DPR; depending on the document aspect ratio, this either throws at the following check or reparses the page with a grossly inflated ratio, breaking assertView for this supported configuration. The calculation must use dimensions corresponding to the captured image or retain whether the source bitmap was full-page.
Useful? React with 👍 / 👎.
| const pixelRatio = imageSize.width / page.viewportSizeInCss.width; | ||
| if (Math.abs(imageSize.height - page.viewportSizeInCss.height * pixelRatio) > 1) { | ||
| throw new Error("Screenshot dimensions do not match the viewport at a consistent pixel ratio"); |
There was a problem hiding this comment.
When the actual screenshot DPR is fractional and differs from the prepared estimate, bitmap width and height are rounded independently, so deriving an exact ratio from width and allowing only one pixel of height error rejects valid images. For example, the repository's Rect.scale rounding maps a 101×201 CSS viewport at DPR 2.625 to 266×528; this code infers 266 / 101 and predicts a height of about 529.37, causing the valid screenshot to throw instead of triggering re-preparation. Compare compatible DPR intervals from both axes or otherwise include the maximum rounding error induced by the viewport aspect ratio.
Useful? React with 👍 / 👎.
KuznetsovRoman
left a comment
KuznetsovRoman
left a comment
There was a problem hiding this comment.
I suggest adding some kind of comment to "css pixels" x "png pixels" comparison so at least it would be clear trade-off.
Besides, /ok
07e460f to
60d0ac1
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60d0ac1db1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (minPixelRatio > maxPixelRatio) { | ||
| // Auto detection uses the estimated DPR and can misidentify the source bitmap. | ||
| if (allowFallback) { | ||
| return getPixelRatioFromImage({ uncroppedSize: imageSize, isFullPage: !isFullPage }, page); |
There was a problem hiding this comment.
In screenshotMode: "auto", the alternate interpretation is attempted only when the first interpretation has inconsistent axis ratios. If the document and viewport have the same aspect ratio, a wrong isFullPage classification is still internally consistent: for example, a 400×800 CSS viewport, an 800×1600 document, and an 800×1600 full-page bitmap initially prepared at DPR 3 are interpreted as a viewport bitmap at DPR 2. The retry then disables further validation and crops/scales using DPR 2 rather than the actual DPR 1, producing an incorrectly framed screenshot. Auto mode needs to detect or safely reject cases where both bitmap interpretations are plausible rather than accepting the first consistent ratio.
Useful? React with 👍 / 👎.
Backport to v8 of this PR: #1331