codeant-ai-for-open-source[bot] commented on code in PR #43755:
URL: https://github.com/apache/superset/pull/43755#discussion_r3903978900
##########
superset/commands/report/execute.py:
##########
@@ -861,9 +865,36 @@ def _get_screenshots(self) -> list[bytes]:
def _get_pdf(self) -> bytes:
"""
- Get chart or dashboard pdf
+ Get chart or dashboard PDF.
+
+ When DASHBOARD_REPORTS_BROWSER_PRINT_PDF is enabled and the report
+ targets a dashboard, attempt the native browser-print path first.
+ Any failure (or flag disabled, or chart report) falls back to the
+ existing screenshot-based path.
+
:raises: ReportSchedulePdfFailedError
"""
+ if (
+ feature_flag_manager.is_feature_enabled(
+ "DASHBOARD_REPORTS_BROWSER_PRINT_PDF"
+ )
+ and self._report_schedule.dashboard # dashboard reports only (not
charts)
+ ):
+ try:
+ pdf_bytes = self._get_browser_print_pdf()
+ if pdf_bytes:
+ logger.info("browser_print_pdf_used %s", self._log_context)
+ return pdf_bytes
Review Comment:
**Suggestion:** The browser-print attempt can consume the report deadline
before failing, leaving insufficient time for `_get_screenshots()` and breaking
the promised fallback. [possible bug]
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Sometimes`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9358675e25d64cc490da90820fa516ad&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=9358675e25d64cc490da90820fa516ad&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/commands/report/execute.py
**Line:** 883:887
**Comment:**
*Possible Bug: The browser-print attempt can consume the report
deadline before failing, leaving insufficient time for `_get_screenshots()` and
breaking the promised fallback.
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%2F43755&comment_hash=5f93e8ddebff69458c908ad02603294afd8d69ce6c84337da72c289cbf686be9&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43755&comment_hash=5f93e8ddebff69458c908ad02603294afd8d69ce6c84337da72c289cbf686be9&reaction=dislike'>๐</a>
##########
superset/utils/webdriver.py:
##########
@@ -979,6 +988,542 @@ def get_screenshot( # pylint: disable=too-many-locals,
too-many-statements # n
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:** An exception during browser setup or authentication occurs
before the `try/finally`, so the newly created context remains open and leaks
browser resources. [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=5fcf83a1166a4fadbb4469046d658a38&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=5fcf83a1166a4fadbb4469046d658a38&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:** 1228:1247
**Comment:**
*Resource Leak: An exception during browser setup or authentication
occurs before the `try/finally`, so the newly created context remains open and
leaks browser resources.
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%2F43755&comment_hash=7bfb26e2bbeb62918fa5a502929c1a85a747bfa34863131c04161968f90b0c51&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43755&comment_hash=7bfb26e2bbeb62918fa5a502929c1a85a747bfa34863131c04161968f90b0c51&reaction=dislike'>๐</a>
##########
superset/utils/webdriver.py:
##########
@@ -979,6 +988,542 @@ def get_screenshot( # pylint: disable=too-many-locals,
too-many-statements # n
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()
+ pdf_bytes: bytes | None = None
+
+ # Determine readiness timeout once (reused for all tabs)
+ load_wait = app.config["SCREENSHOT_LOAD_WAIT"]
+ if report_execution_context:
+ effective_wait = report_execution_context.deadline.timeout_seconds(
+ "chart_readiness",
+
reserve_seconds=report_execution_context.readiness_reserve_seconds,
+ )
Review Comment:
**Suggestion:** `effective_wait` is reused independently for navigation,
mounting, and readiness, so one PDF attempt can consume several full report
timeouts before fallback begins. [possible bug]
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Sometimes`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4f15ed76d10c4d1b835160fb51fbbdd8&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=4f15ed76d10c4d1b835160fb51fbbdd8&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:** 1253:1256
**Comment:**
*Possible Bug: `effective_wait` is reused independently for navigation,
mounting, and readiness, so one PDF attempt can consume several full report
timeouts before fallback begins.
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%2F43755&comment_hash=6b28e2aaa36a0146985c63d18e663ee064d48743c8b2c4b400913b6ee9867871&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43755&comment_hash=6b28e2aaa36a0146985c63d18e663ee064d48743c8b2c4b400913b6ee9867871&reaction=dislike'>๐</a>
##########
superset/utils/screenshot_utils.py:
##########
@@ -223,6 +223,1084 @@ class
TiledScreenshotBudgetExceededError(ScreenshotTaskBudgetExceededError):
f"() => {{ {UNREADY_CHART_HOLDERS_JS_BODY} "
"return holders.length > 0 && unready.length === 0; }"
)
+
+# 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) {{
+ const chartIdMatch =
holder.className.match(/{CHART_ID_CLASS_PATTERN}/);
+ unready.push({{ chartId: chartIdMatch ? chartIdMatch[1] : null }});
+ }}
+ }}
+"""
+
+PRINT_ALL_CHART_HOLDERS_READY_JS = (
+ f"() => {{ {PRINT_ALL_CHART_HOLDERS_READY_JS_BODY} "
+ "return holders.length > 0 && unready.length === 0; }"
Review Comment:
**Suggestion:** This predicate never succeeds when a dashboard has no chart
holders, so markdown-only or empty dashboards wait until timeout instead of
producing a native PDF. [incorrect condition logic]
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Sometimes`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4391ff8c24d64484812099c702eb3e2c&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=4391ff8c24d64484812099c702eb3e2c&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:** 249:249
**Comment:**
*Incorrect Condition Logic: This predicate never succeeds when a
dashboard has no chart holders, so markdown-only or empty dashboards wait until
timeout instead of producing a native PDF.
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%2F43755&comment_hash=6183114f8cb6e1026ebfc96831a4097402c427b6010b98195cda0c49b93530eb&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43755&comment_hash=6183114f8cb6e1026ebfc96831a4097402c427b6010b98195cda0c49b93530eb&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]
