codeant-ai-for-open-source[bot] commented on code in PR #42118:
URL: https://github.com/apache/superset/pull/42118#discussion_r3599301065
##########
tests/unit_tests/utils/test_screenshot_utils.py:
##########
@@ -397,3 +398,106 @@ def test_animation_wait_default_is_zero(self):
sig = inspect.signature(take_tiled_screenshot)
assert sig.parameters["animation_wait"].default == 0
+
+
+class TestTileWaitBudget:
+ @pytest.fixture
+ def mock_page(self):
Review Comment:
**Suggestion:** Add explicit parameter and return type annotations to this
fixture method signature. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
The new fixture method omits both parameter and return type annotations,
which violates the Python type-hints rule for newly added code.
</details>
<details>
<summary><b>Rule source ๐ </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7b38d85cc459435fb20d574556994ca5&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=7b38d85cc459435fb20d574556994ca5&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/utils/test_screenshot_utils.py
**Line:** 405:405
**Comment:**
*Custom Rule: Add explicit parameter and return type annotations to
this fixture method signature.
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%2F42118&comment_hash=809943a70e337a20fbfc77808b8ffea53f6a9596232755f9a521be90ced6ef62&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42118&comment_hash=809943a70e337a20fbfc77808b8ffea53f6a9596232755f9a521be90ced6ef62&reaction=dislike'>๐</a>
##########
superset/utils/screenshot_utils.py:
##########
@@ -28,6 +29,35 @@
# Time to wait after scrolling for content to settle and load (in milliseconds)
SCROLL_SETTLE_TIMEOUT_MS = 1000
+# Total wall-clock budget, in seconds, for the entire tiled-screenshot
+# operation (element lookup plus all per-tile spinner/animation waits
+# combined). Each tile's wait is capped at whatever remains of this budget
+# so a slow dashboard degrades gracefully instead of running past the
+# Celery task time limit and getting SIGKILLed mid-capture.
+#
+# This is a conservative fixed constant rather than a value derived from
+# Celery's configured task time limit at runtime, because that limit isn't
+# reliably reachable from here: superset/tasks/scheduler.py sets it
+# per-report-schedule via `apply_async(time_limit=..., soft_time_limit=...)`
+# only when a schedule defines `working_timeout`
+# (ALERT_REPORTS_WORKING_TIME_OUT_KILL); there is no static config value
+# that always reflects the limit actually enforced, and this function can
+# also run outside of a Celery task entirely (e.g. synchronous thumbnail
+# generation).
+#
+# Production has observed a Celery hard task_time_limit of 1740s (29 min)
+# for report execution (2026-07-13 incident: a tiled screenshot was killed
+# mid-capture with SoftTimeLimitExceeded). This budget leaves a 300s margin
+# under that ceiling for the rest of the pipeline that runs after tiling
+# completes: combining tiles into one image, building the PDF, and
+# uploading/delivering the notification.
+TILED_SCREENSHOT_TOTAL_WAIT_BUDGET_SECONDS = 1440 # 1740s limit - 300s margin
Review Comment:
**Suggestion:** Add an explicit type annotation to this new module-level
constant (for example, annotate it as an integer) to satisfy the type-hint
requirement for relevant variables. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
This is a new module-level constant introduced without a type annotation,
and the custom rule requires type hints for relevant variables that can be
annotated. An `int` annotation would satisfy the rule.
</details>
<details>
<summary><b>Rule source ๐ </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=968703c5289f4b0a8e831cc1c71991bc&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=968703c5289f4b0a8e831cc1c71991bc&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<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:** 54:54
**Comment:**
*Custom Rule: Add an explicit type annotation to this new module-level
constant (for example, annotate it as an integer) to satisfy the type-hint
requirement for relevant variables.
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%2F42118&comment_hash=da7aaf4f7698a094c3f4e87e27fd64b35811c2a91914d43f3602e3b8034acc10&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42118&comment_hash=da7aaf4f7698a094c3f4e87e27fd64b35811c2a91914d43f3602e3b8034acc10&reaction=dislike'>๐</a>
##########
tests/unit_tests/utils/test_screenshot_utils.py:
##########
@@ -397,3 +398,106 @@ def test_animation_wait_default_is_zero(self):
sig = inspect.signature(take_tiled_screenshot)
assert sig.parameters["animation_wait"].default == 0
+
+
+class TestTileWaitBudget:
+ @pytest.fixture
+ def mock_page(self):
+ """Create a mock Playwright page object for a 3-tile (5000px)
dashboard."""
+ page = MagicMock()
+ element = MagicMock()
+ page.locator.return_value = element
+ page.evaluate.return_value = {
+ "height": 5000,
+ "top": 100,
+ "left": 50,
+ "width": 800,
+ }
+ page.screenshot.return_value = b"fake_screenshot_data"
+ return page
+
+ def test_per_tile_wait_shrinks_as_budget_depletes(self, mock_page,
monkeypatch):
Review Comment:
**Suggestion:** Add type annotations for all parameters and an explicit
return type on this new test method. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
The added test method has unannotated parameters and no return type
annotation, so it matches the type-hints violation rule.
</details>
<details>
<summary><b>Rule source ๐ </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f20f628fd0114eca86e2609f05bd71c8&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=f20f628fd0114eca86e2609f05bd71c8&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/utils/test_screenshot_utils.py
**Line:** 419:419
**Comment:**
*Custom Rule: Add type annotations for all parameters and an explicit
return type on this new test method.
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%2F42118&comment_hash=a9f2206e01e67d3c1c64a79a1454d100716b66a8ea0dc3208814eeebc6f008a1&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42118&comment_hash=a9f2206e01e67d3c1c64a79a1454d100716b66a8ea0dc3208814eeebc6f008a1&reaction=dislike'>๐</a>
##########
tests/unit_tests/utils/test_screenshot_utils.py:
##########
@@ -397,3 +398,106 @@ def test_animation_wait_default_is_zero(self):
sig = inspect.signature(take_tiled_screenshot)
assert sig.parameters["animation_wait"].default == 0
+
+
+class TestTileWaitBudget:
+ @pytest.fixture
+ def mock_page(self):
+ """Create a mock Playwright page object for a 3-tile (5000px)
dashboard."""
+ page = MagicMock()
+ element = MagicMock()
+ page.locator.return_value = element
+ page.evaluate.return_value = {
+ "height": 5000,
+ "top": 100,
+ "left": 50,
+ "width": 800,
+ }
+ page.screenshot.return_value = b"fake_screenshot_data"
+ return page
+
+ def test_per_tile_wait_shrinks_as_budget_depletes(self, mock_page,
monkeypatch):
+ """Each tile's spinner-wait timeout is capped at the remaining
budget."""
+ monkeypatch.setattr(
+
"superset.utils.screenshot_utils.TILED_SCREENSHOT_TOTAL_WAIT_BUDGET_SECONDS",
# noqa: E501
+ 1000,
+ )
+ # monotonic() is called once for start_time, then once per tile to
+ # compute elapsed/remaining budget before that tile's spinner wait.
+ monotonic_values = iter([0, 0, 950, 990])
+ with patch(
+ "superset.utils.screenshot_utils.time.monotonic",
+ side_effect=lambda: next(monotonic_values),
+ ):
+ with
patch("superset.utils.screenshot_utils.combine_screenshot_tiles"):
+ result = take_tiled_screenshot(
+ mock_page, "dashboard", tile_height=2000, load_wait=100
+ )
+
+ assert result is not None
+ timeouts = [
+ call[1]["timeout"] for call in
mock_page.wait_for_function.call_args_list
+ ]
+ # remaining budget: 1000, 50, 10 seconds -> capped timeouts shrink
+ assert timeouts == [100 * 1000, 50 * 1000, 10 * 1000]
+ assert timeouts == sorted(timeouts, reverse=True)
+
+ def test_budget_exhausted_raises_and_stops_capturing(self, mock_page,
monkeypatch):
+ """Exhausting the budget aborts cleanly instead of capturing
unchecked."""
+ monkeypatch.setattr(
+
"superset.utils.screenshot_utils.TILED_SCREENSHOT_TOTAL_WAIT_BUDGET_SECONDS",
# noqa: E501
+ 1000,
+ )
+ # start_time=0, tile 0 check: elapsed=0 (proceeds and captures),
+ # tile 1 check: elapsed=1000 -> remaining=0 -> raise before capturing.
+ monotonic_values = iter([0, 0, 1000])
+ with patch(
+ "superset.utils.screenshot_utils.time.monotonic",
+ side_effect=lambda: next(monotonic_values),
+ ):
+ with patch(
+ "superset.utils.screenshot_utils.combine_screenshot_tiles"
+ ) as mock_combine:
+ with patch("superset.utils.screenshot_utils.logger") as
mock_logger:
+ with pytest.raises(TiledScreenshotBudgetExceededError):
+ take_tiled_screenshot(
+ mock_page, "dashboard", tile_height=2000,
load_wait=100
+ )
+
+ # Only the first tile was captured before the budget ran out.
+ assert mock_page.screenshot.call_count == 1
+ # Tiles were never combined -- the function raised before that point.
+ mock_combine.assert_not_called()
+
+ mock_logger.error.assert_called_once()
+ error_args = mock_logger.error.call_args[0]
+ assert "budget exhausted" in error_args[0]
+ # tiles captured, tiles total, elapsed seconds, budget seconds
+ assert error_args[1] == 1
+ assert error_args[2] == 3
+ assert error_args[3] == 1000
+ assert error_args[4] == 1000
+
+ def test_fast_dashboard_matches_default_behavior(self, mock_page):
Review Comment:
**Suggestion:** Add parameter and return type annotations to this test
method declaration. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
This new test method omits type hints for its argument and return value, so
the suggestion correctly identifies a type-hints violation.
</details>
<details>
<summary><b>Rule source ๐ </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=6de36cb75c8447d7b86ccff9fe4602ac&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=6de36cb75c8447d7b86ccff9fe4602ac&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/utils/test_screenshot_utils.py
**Line:** 481:481
**Comment:**
*Custom Rule: Add parameter and return type annotations to this test
method declaration.
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%2F42118&comment_hash=57588b945dc2632369429cf0ff516110c40c922e4d36f1ae8add364cf23ca0fa&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42118&comment_hash=57588b945dc2632369429cf0ff516110c40c922e4d36f1ae8add364cf23ca0fa&reaction=dislike'>๐</a>
##########
tests/unit_tests/utils/test_screenshot_utils.py:
##########
@@ -397,3 +398,106 @@ def test_animation_wait_default_is_zero(self):
sig = inspect.signature(take_tiled_screenshot)
assert sig.parameters["animation_wait"].default == 0
+
+
+class TestTileWaitBudget:
+ @pytest.fixture
+ def mock_page(self):
+ """Create a mock Playwright page object for a 3-tile (5000px)
dashboard."""
+ page = MagicMock()
+ element = MagicMock()
+ page.locator.return_value = element
+ page.evaluate.return_value = {
+ "height": 5000,
+ "top": 100,
+ "left": 50,
+ "width": 800,
+ }
+ page.screenshot.return_value = b"fake_screenshot_data"
+ return page
+
+ def test_per_tile_wait_shrinks_as_budget_depletes(self, mock_page,
monkeypatch):
+ """Each tile's spinner-wait timeout is capped at the remaining
budget."""
+ monkeypatch.setattr(
+
"superset.utils.screenshot_utils.TILED_SCREENSHOT_TOTAL_WAIT_BUDGET_SECONDS",
# noqa: E501
+ 1000,
+ )
+ # monotonic() is called once for start_time, then once per tile to
+ # compute elapsed/remaining budget before that tile's spinner wait.
+ monotonic_values = iter([0, 0, 950, 990])
+ with patch(
+ "superset.utils.screenshot_utils.time.monotonic",
+ side_effect=lambda: next(monotonic_values),
+ ):
+ with
patch("superset.utils.screenshot_utils.combine_screenshot_tiles"):
+ result = take_tiled_screenshot(
+ mock_page, "dashboard", tile_height=2000, load_wait=100
+ )
+
+ assert result is not None
+ timeouts = [
+ call[1]["timeout"] for call in
mock_page.wait_for_function.call_args_list
+ ]
+ # remaining budget: 1000, 50, 10 seconds -> capped timeouts shrink
+ assert timeouts == [100 * 1000, 50 * 1000, 10 * 1000]
+ assert timeouts == sorted(timeouts, reverse=True)
+
+ def test_budget_exhausted_raises_and_stops_capturing(self, mock_page,
monkeypatch):
Review Comment:
**Suggestion:** Add type hints to this method signature, including fixture
argument types and a return type. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
This newly added test method is missing type annotations on its parameters
and return type, which is a real violation of the type-hints rule.
</details>
<details>
<summary><b>Rule source ๐ </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=8dae0036fa864eefb48dfb6676e4a31a&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=8dae0036fa864eefb48dfb6676e4a31a&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/utils/test_screenshot_utils.py
**Line:** 445:445
**Comment:**
*Custom Rule: Add type hints to this method signature, including
fixture argument types and a return type.
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%2F42118&comment_hash=3de3388727e50825725e25aae3d21fbf3211ccdd9c76403f22557ff14ef7c776&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42118&comment_hash=3de3388727e50825725e25aae3d21fbf3211ccdd9c76403f22557ff14ef7c776&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]
