fix(render): viewport not set when only one dimension specified — causes 2x width in generated images#13
Conversation
…set when only one dimension is specified The condition 'if viewport_width is not None and viewport_height is not None' required BOTH dimensions to be present before calling set_viewport_size(). When only width was specified (e.g., from HTML meta tag 'width=630' without a height), or when neither was specified (falling back to defaults 800x720), the viewport was never set, causing Playwright to use its internal default of 1280x720. This caused generated images to be ~2x wider than expected in plugins like astrbot_plugin_bilibili (issues #92, #95), where templates designed for 630px viewport were rendered at 1280px, then multiplied by device_scale_factor 'ultra' (1.8x). The fix removes the conditional guard entirely, since set_viewport_size() now always receives properly resolved values (from options > HTML meta > defaults).
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Now that
set_viewport_sizeis called unconditionally, the behavior also changes for the case where bothviewport_widthandviewport_heightareNone; consider adding a short comment above this block to explicitly document that we intentionally always set a viewport (using 800x720 defaults) rather than relying on Playwright’s default.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Now that `set_viewport_size` is called unconditionally, the behavior also changes for the case where both `viewport_width` and `viewport_height` are `None`; consider adding a short comment above this block to explicitly document that we intentionally always set a viewport (using 800x720 defaults) rather than relying on Playwright’s default.
## Individual Comments
### Comment 1
<location path="src/render.py" line_range="241-242" />
<code_context>
# Set viewport size if either width or height is specified
- if viewport_width is not None and viewport_height is not None:
- # Default values if one dimension not specified
- await page.set_viewport_size({"width": width, "height": height})
- logger.info(f"html2pic: set viewport size to {width}x{height}")
+ # Default values if one dimension not specified
+ await page.set_viewport_size({"width": width, "height": height})
</code_context>
<issue_to_address>
**question (bug_risk):** Consider whether forcing a default viewport even when no dimensions are provided changes existing behavior unintentionally.
Previously, the viewport was only set when both dimensions were provided; now it’s always set to 800×720, even when both are `None`. This could break callers that relied on the browser’s default. If that change isn’t intentional, consider guarding with something like `if viewport_width is not None or viewport_height is not None:` so the default is only applied when at least one dimension is specified.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| logger.info(f"html2pic: set viewport size to {width}x{height}") | ||
|
|
There was a problem hiding this comment.
question (bug_risk): Consider whether forcing a default viewport even when no dimensions are provided changes existing behavior unintentionally.
Previously, the viewport was only set when both dimensions were provided; now it’s always set to 800×720, even when both are None. This could break callers that relied on the browser’s default. If that change isn’t intentional, consider guarding with something like if viewport_width is not None or viewport_height is not None: so the default is only applied when at least one dimension is specified.
There was a problem hiding this comment.
Code Review
This pull request modifies the html2pic function in src/render.py to set the viewport size unconditionally, ensuring default dimensions are applied when overrides are not provided. Feedback was provided to update the surrounding comments, as the removal of the conditional logic rendered the existing documentation misleading or redundant.
| # Default values if one dimension not specified | ||
| await page.set_viewport_size({"width": width, "height": height}) | ||
| logger.info(f"html2pic: set viewport size to {width}x{height}") |
There was a problem hiding this comment.
The comments in this block are now misleading because the if condition was removed, meaning the viewport is now set unconditionally. The comment on line 239 is redundant as the default logic is already handled in lines 236-237. Furthermore, the comment on line 238 (which is a context line in this diff) now contradicts the implementation as it states the viewport is only set if a dimension is specified. Consider updating the comments to accurately reflect that the viewport is now always set to ensure the library's default dimensions (800x720) are applied when no overrides are provided.
| # Default values if one dimension not specified | |
| await page.set_viewport_size({"width": width, "height": height}) | |
| logger.info(f"html2pic: set viewport size to {width}x{height}") | |
| # Set viewport size using specified values or defaults | |
| await page.set_viewport_size({"width": width, "height": height}) | |
| logger.info(f"html2pic: set viewport size to {width}x{height}") |
Summary
Summary by Sourcery
Bug Fixes: