codeant-ai-for-open-source[bot] commented on code in PR #43762:
URL: https://github.com/apache/superset/pull/43762#discussion_r3904450543
##########
superset/utils/screenshot_utils.py:
##########
@@ -345,6 +345,1083 @@ def _unready_chart_holders_js_body(*, viewport_only:
bool) -> str:
}}
"""
+# Like REPORT_CHART_HOLDERS_READY_JS, but scoped to ALL chart holders, not just
+# viewport-visible ones. Required for browser-print mode where page.pdf()
+# renders the full DOM. The getBoundingClientRect() viewport filter from
+# UNREADY_CHART_HOLDERS_JS_BODY is intentionally absent here.
+PRINT_ALL_CHART_HOLDERS_READY_JS_BODY = f"""
+ const holders = document.querySelectorAll('{CHART_HOLDER_SELECTOR}');
+ const unready = [];
+ for (const holder of holders) {{
+ const hasSliceContainer = holder.querySelector(
+ '{SLICE_CONTAINER_SELECTOR}'
+ ) !== null;
+ const stillLoading = holder.querySelector('{LOADING_SELECTOR}') !==
null;
+ const isReady = holder.querySelector('{TERMINAL_MARKER_SELECTOR}') !==
null;
+ if (stillLoading || !isReady) {{
Review Comment:
**Suggestion:** `slice_container` is treated as terminal without checking
ECharts paint completion, so PDFs can capture charts whose canvases are still
blank. [logic error]
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Sometimes`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=decfd75deeaf438d8b95cd966b8d016d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=decfd75deeaf438d8b95cd966b8d016d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/utils/screenshot_utils.py
**Line:** 360:361
**Comment:**
*Logic Error: `slice_container` is treated as terminal without checking
ECharts paint completion, so PDFs can capture charts whose canvases are still
blank.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43762&comment_hash=dbc534da2c6a63e8357c46d88465ade43f89dded903bbafdcd9c6343f6faf069&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43762&comment_hash=dbc534da2c6a63e8357c46d88465ade43f89dded903bbafdcd9c6343f6faf069&reaction=dislike'>๐</a>
##########
superset/utils/webdriver.py:
##########
@@ -986,3 +995,539 @@ def get_screenshot( # pylint: disable=too-many-locals,
too-many-statements # n
finally:
context.close()
return img
+
+ @staticmethod
+ def _escape_html(text: str) -> str:
+ """HTML-escape a plain-text string for safe inline HTML embedding."""
+ return (
+ text.replace("&", "&")
+ .replace("<", "<")
+ .replace(">", ">")
+ .replace('"', """)
+ )
+
+ @staticmethod
+ def _resolve_slot(
+ raw: str,
+ title: str,
+ ) -> str:
+ """
+ Expand user-defined token placeholders in a header/footer slot string.
+
+ Supported tokens:
+ {title} โ the dashboard title (HTML-escaped)
+ {date} โ replaced with a <span class="date"></span> element so
+ Chromium injects the actual print date at render time.
+
+ The returned string is safe for direct insertion into an inline-HTML
+ Playwright template (all literal text is HTML-escaped; only the
+ Chromium-class <span> elements are allowed through unescaped).
+ """
+ # Split on {date} first so we can handle it as a Chromium span.
+ # Everything else: substitute {title} then HTML-escape the result,
+ # so a dashboard title containing "<", ">" or "&" cannot inject markup.
+ parts = raw.split("{date}")
+ resolved_parts = []
+ for i, part in enumerate(parts):
+ safe_part =
WebDriverPlaywright._escape_html(part.replace("{title}", title))
+ resolved_parts.append(safe_part)
+ if i < len(parts) - 1:
+ resolved_parts.append('<span class="date"></span>')
+ return "".join(resolved_parts)
+
+ @staticmethod
+ def _slot_span(content: str, extra_style: str = "") -> str:
+ """
+ Wrap resolved slot content in a flex-child <span> with overflow
+ protection so long strings are truncated with an ellipsis rather
+ than spilling into adjacent slots or off the page band.
+
+ Each slot is capped at 200px (paper pixels โ templates are rendered
+ at full paper width, not scaled by page.pdf(scale)).
+ """
+ base = (
+ "display:inline-block;"
+ "max-width:200px;"
+ "overflow:hidden;"
+ "text-overflow:ellipsis;"
+ "white-space:nowrap;"
+ "vertical-align:bottom;"
+ )
+ style = base + extra_style
+ return f'<span style="{style}">{content}</span>'
+
+ @staticmethod
+ def _build_pdf_header_template(
+ title: str,
+ content: dict[str, str] | None = None,
+ ) -> str:
+ """
+ Build the Playwright header_template HTML string.
+
+ Rules for Playwright/Chromium header templates:
+ - Must be a single root element.
+ - All styles must be inline โ no <style> tags.
+ - font-size defaults to 0px; must be set explicitly or text is
invisible.
+ - Special classes injected by Chromium: date, title, url,
+ pageNumber, totalPages.
+ - Template is rendered at full paper width, independent of
page.pdf(scale).
+ - Lives entirely inside the top margin space.
+
+ ``content`` is a dict with optional keys "left", "center", "right"
+ whose values are plain-text strings supporting {title} and {date}
+ tokens (see _resolve_slot). Defaults to the built-in layout when
+ None or when a key is absent.
+ """
+ _c = content or {}
+ raw_left = _c.get("left", "{title}")
+ raw_center = _c.get("center", "")
+ raw_right = _c.get("right", "Apache Superset | {date}")
+
+ left_html = WebDriverPlaywright._slot_span(
+ WebDriverPlaywright._resolve_slot(raw_left, title),
+ "font-weight:700;font-size:11px;letter-spacing:0.2px;",
+ )
+ center_html = WebDriverPlaywright._slot_span(
+ WebDriverPlaywright._resolve_slot(raw_center, title),
+ "font-size:8px;color:#57606a;",
+ )
+ right_html = WebDriverPlaywright._slot_span(
+ WebDriverPlaywright._resolve_slot(raw_right, title),
+ "font-size:8px;color:#57606a;text-align:right;",
+ )
+
+ return (
+ '<div style="'
+ "width:100%;"
+ "font-family:Arial,Helvetica,sans-serif;"
+ "font-size:9px;"
+ "color:#1f2328;"
+ "display:flex;"
+ "justify-content:space-between;"
+ "align-items:flex-end;"
+ "padding:0 10mm 4px 10mm;"
+ "box-sizing:border-box;"
+ "border-bottom:1.5px solid #3b82d4;"
+ '">'
+ f"{left_html}"
+ f"{center_html}"
+ f"{right_html}"
+ "</div>"
+ )
+
+ @staticmethod
+ def _build_pdf_footer_template(
+ content: dict[str, str] | None = None,
+ ) -> str:
+ """
+ Build the Playwright footer_template HTML string.
+
+ The right slot is always "Page N of M" using Chromium's special
+ pageNumber / totalPages classes substituted at render time. It
+ cannot be overridden via ``content``.
+
+ ``content`` is a dict with optional keys "left" and "center" whose
+ values are plain-text strings supporting {title} and {date} tokens.
+ Defaults to the built-in layout when None or when a key is absent.
+ """
+ _c = content or {}
+ raw_left = _c.get("left", "Confidential")
+ raw_center = _c.get("center", "Generated by Apache Superset")
+
+ left_html = WebDriverPlaywright._slot_span(
+ WebDriverPlaywright._resolve_slot(raw_left, ""),
+ )
+ center_html = WebDriverPlaywright._slot_span(
+ WebDriverPlaywright._resolve_slot(raw_center, ""),
+ "text-align:center;",
+ )
+ # Right slot: fixed page numbering โ not user-overridable.
+ page_html = (
+ '<span style="white-space:nowrap;">'
+ 'Page <span class="pageNumber"></span>'
+ ' of <span class="totalPages"></span>'
+ "</span>"
+ )
+
+ return (
+ '<div style="'
+ "width:100%;"
+ "font-family:Arial,Helvetica,sans-serif;"
+ "font-size:8px;"
+ "color:#57606a;"
+ "display:flex;"
+ "justify-content:space-between;"
+ "align-items:flex-start;"
+ "padding:4px 10mm 0 10mm;"
+ "box-sizing:border-box;"
+ "border-top:1px solid #e5e7eb;"
+ '">'
+ f"{left_html}"
+ f"{center_html}"
+ f"{page_html}"
+ "</div>"
+ )
+
+ def get_print_pdf( # noqa: C901
+ self,
+ url: str,
+ user: "User | None" = None,
+ log_context: str | None = None,
+ report_execution_context: ReportExecutionContext | None = None,
+ header_title: str | None = None,
+ font_size: str | None = None,
+ print_layout: str | None = None,
+ print_orientation: str | None = None,
+ tab_ids: list[str] | None = None,
+ header_content: dict[str, str] | None = None,
+ footer_content: dict[str, str] | None = None,
+ ) -> bytes | None:
+ """
+ Render the dashboard in print-ready mode and call page.pdf().
+
+ When tab_ids is provided (list of Superset TAB-xxx component IDs),
+ the dashboard is rendered once per tab by appending #TAB-xxx to the
+ URL โ each navigation activates that tab so its charts mount and
render.
+ The resulting per-tab PDFs are merged into a single document via pypdf.
+ Falls back to single-URL rendering if pypdf is not available.
+
+ Uses PRINT_ALL_CHART_HOLDERS_READY_JS (all holders, not just
+ viewport-visible) to detect readiness, then calls Playwright's
+ native page.pdf() instead of page.screenshot().
+
+ The viewport width is set to the authored dashboard width (default
+ 1600 px) so ECharts/canvas elements measure and draw at their design
+ resolution. page.pdf(scale=794/1600) maps the content onto A4 paper
+ width without blank guttering.
+
+ When header_title is provided (and BROWSER_PRINT_PDF_HEADER_FOOTER is
+ True in app config), Playwright's display_header_footer API is used to
+ stamp a title+date header and a confidential/page-count footer on every
+ page. The header/footer template is rendered at full paper width by
the
+ Chromium print engine and is NOT affected by page.pdf(scale).
+
+ font_size ('small' | 'medium' | None) controls DOM-rendered text sizes.
+ Big Number charts use an inline style for font-size which CSS
!important
+ cannot override; SET_PRINT_FONT_SIZE_JS patches those inline styles
+ directly before page.pdf() is called.
+
+ print_layout ('2col' | None) enables two-column adaptive layout:
+ ANNOTATE_PRINT_COLUMNS_JS is called before page.pdf() to tag each
+ .dragdroppable-column with data-print-col-span="half"|"full" based on
+ its original pixel width relative to its row. The CSS injected via
+ ?print_layout=2col in the URL then uses those attributes to lay out
+ small charts side-by-side. Table charts are always forced full-width
+ by the JS annotation regardless of their original size.
+
+ print_orientation controls page rotation:
+ 'portrait' (default/None) โ A4 portrait throughout.
+ 'landscape' โ page.pdf(landscape=True) entire document landscape.
+ 'auto' โ CSS @page named pages + prefer_css_page_size=True.
+ Wide tables get data-print-landscape="true" set by
+ SCALE_WIDE_TABLES_JS and render in landscape; all
+ other pages stay portrait.
+
+ Returns None (never raises) so the caller can fall back to
+ the existing screenshot path.
+ """
+ if not PLAYWRIGHT_AVAILABLE:
+ return None
+ browser_args = app.config["WEBDRIVER_OPTION_ARGS"]
+ browser = _browser_manager.get_browser(browser_args)
+ pixel_density = app.config["WEBDRIVER_WINDOW"].get("pixel_density", 1)
+ # Render at the authored dashboard width (default 1600 px) so
+ # ECharts/canvas elements measure and draw at their design resolution.
+ # page.pdf(scale=...) then scales the rendered content down to fit
+ # A4 paper width (794 px at 96 dpi), giving full-resolution charts
+ # with no blank guttering โ equivalent to browser print-to-PDF with
+ # a custom scale factor.
+ pdf_viewport_width = app.config.get(
+ "BROWSER_PRINT_PDF_VIEWPORT_WIDTH", self._window[0]
+ )
+ context = browser.new_context(
+ bypass_csp=True,
+ viewport={"height": self._window[1], "width": pdf_viewport_width},
+ device_scale_factor=pixel_density,
+ )
+
context.set_default_timeout(app.config["SCREENSHOT_PLAYWRIGHT_DEFAULT_TIMEOUT"])
+ if user:
+ self.auth(user, context)
+ page = context.new_page()
Review Comment:
**Suggestion:** Exceptions during context setup, authentication, or page
creation occur before the cleanup block, leaving the Playwright context open on
repeated report failures. [resource leak]
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Sometimes`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=add8ef2b35494be69c323e31d0a2bd40&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=add8ef2b35494be69c323e31d0a2bd40&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/utils/webdriver.py
**Line:** 1247:1255
**Comment:**
*Resource Leak: Exceptions during context setup, authentication, or
page creation occur before the cleanup block, leaving the Playwright context
open on repeated report failures.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43762&comment_hash=1c33a98f29f2cabb885ae4d345b308491a4eaf5c4b9e4549c3bd1c2628095d89&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43762&comment_hash=1c33a98f29f2cabb885ae4d345b308491a4eaf5c4b9e4549c3bd1c2628095d89&reaction=dislike'>๐</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
