bito-code-review[bot] commented on code in PR #44465: URL: https://github.com/apache/superset/pull/44465#discussion_r4062016943
########## docs/admin_docs/configuration/alerts-reports.mdx: ########## @@ -478,6 +478,37 @@ There are many reasons that reports might not be working. Try these steps to che ### Blank PDF or PNG captures +Final screenshot bytes are validated independently of the capture implementation. +PDF delivery requires evidence that the exact PDF was assembled from validated +images during this execution. Capture rejection remains sticky for the execution; +substituting another attachment cannot clear it. Detection is a heuristic, not a +guarantee that every chart contains correct data. Uniform fills are rejected +regardless of colour or theme. A small label or logo does not override a blank +classification. Extremely sparse legitimate output can therefore be rejected; +a solid-fill KPI without visible structure has no pixel-only exemption. + +Chart error and empty states are terminal for readiness, but a rendered error is +not successful chart data. Logs distinguish `rendered_holders`, `empty_holders`, +`error_holders` and `semantic_success`; `report_semantic_status` warns when a +completed capture includes terminal chart errors. Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Ambiguous terminal wording</b></div> <div id="fix"> 'Chart error and empty states are terminal for readiness' is ambiguous: 'terminal' suggests capture fails, but lines 498-499 say recognized terminal chart errors are 'capture-ready and may be delivered with a warning'. Code agrees errors only warn (`report_semantic_status` at `screenshot_utils.py:1642-1659`). Please reword to say the readiness wait stops without retry, not that delivery is blocked. </div> </div> <small><i>Code Review Run #b4ab75</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## docs/admin_docs/configuration/alerts-reports.mdx: ########## @@ -478,6 +478,37 @@ There are many reasons that reports might not be working. Try these steps to che ### Blank PDF or PNG captures +Final screenshot bytes are validated independently of the capture implementation. +PDF delivery requires evidence that the exact PDF was assembled from validated +images during this execution. Capture rejection remains sticky for the execution; +substituting another attachment cannot clear it. Detection is a heuristic, not a +guarantee that every chart contains correct data. Uniform fills are rejected +regardless of colour or theme. A small label or logo does not override a blank +classification. Extremely sparse legitimate output can therefore be rejected; +a solid-fill KPI without visible structure has no pixel-only exemption. + +Chart error and empty states are terminal for readiness, but a rendered error is +not successful chart data. Logs distinguish `rendered_holders`, `empty_holders`, +`error_holders` and `semantic_success`; `report_semantic_status` warns when a +completed capture includes terminal chart errors. + +This is not a guarantee that every delivered image contains useful data. A large +enough title or logo can still exceed the detector's content thresholds while the +rest of the page is incomplete; removing the sparse-content override does not +solve that general limitation. Recognized terminal chart errors are capture-ready +and may be delivered with a warning, provided the +image passes visual validation. Operators requiring all charts to contain data +must not treat this policy as that guarantee. The strict invariant is that an Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Triplicated disclaimer text</b></div> <div id="fix"> The same 'not a guarantee' caveat is stated three times in the added text: lines 484-485 ('Detection is a heuristic, not a guarantee...'), 495-496 ('This is not a guarantee that every delivered image contains useful data'), and 500-501 ('Operators requiring all charts to contain data must not treat this policy as that guarantee'). Consolidate into one statement to keep the troubleshooting section scannable. </div> </div> <small><i>Code Review Run #b4ab75</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## tests/unit_tests/utils/test_screenshot_utils.py: ########## @@ -90,6 +96,45 @@ def test_stable_readiness_skips_when_budget_below_polling_margin() -> None: page.wait_for_function.assert_not_called() +def test_holder_diagnostics_include_offscreen_errors_for_full_capture() -> None: Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Missing test docstring</b></div> <div id="fix"> New test function lacks a docstring, which BITO.md adaptive rule 12148 requires for every newly added test function; sibling helpers in this file (e.g. `_report_context`, line 431) include one. Add a brief docstring stating the scenario: full-capture diagnostics must report `virtualized` for off-screen holders and `error` for alert holders. </div> </div> <small><i>Code Review Run #b4ab75</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## tests/unit_tests/utils/test_report_execution.py: ########## @@ -136,6 +180,23 @@ def test_capture_rejection_is_sticky_for_the_execution() -> None: ) +def test_artifact_approval_is_namespaced_by_kind() -> None: + context = ReportExecutionContext( + execution_id=UUID("084e7ee6-5557-4ecd-9632-b7f39c9ec524"), + report_schedule_id=7, + deadline=ReportExecutionDeadline(total_seconds=900), + ) + artifact = b"same exact bytes" Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Missing local type annotation</b></div> <div id="fix"> Local `artifact` lacks an explicit type annotation. Repo rule [13153] requires explicit annotations for all local variables in test files, even when inferable; the surrounding test code is otherwise fully typed. ([BITO.md adaptive rule 13153]) </div> </div> <small><i>Code Review Run #b4ab75</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## tests/unit_tests/utils/test_report_execution.py: ########## @@ -44,6 +46,48 @@ def _report_config(**overrides: int) -> dict[str, int | bool]: return config +def test_chart_holder_diagnostics_separate_terminal_errors_from_success() -> None: + diagnostics = ChartHolderDiagnostics.from_holder_states( + [ + {"chartId": "1", "state": "rendered"}, + {"chartId": "2", "state": "rendered"}, + {"chartId": "3", "state": "empty"}, + {"chartId": "4", "state": "error"}, + {"chartId": "5", "state": "virtualized"}, + ] + ) + + assert diagnostics.mounted_holders == 5 + assert diagnostics.ready_holders == 5 + assert diagnostics.rendered_holders == 2 + assert diagnostics.empty_holders == 1 + assert diagnostics.error_holders == 1 + assert diagnostics.virtualized_holders == 1 + assert diagnostics.unready_holders == 0 + assert diagnostics.semantic_success is False + + +def test_chart_holder_diagnostics_count_unready_holders() -> None: + diagnostics = ChartHolderDiagnostics.from_holder_states( + [ + {"chartId": "1", "state": "rendered"}, + {"chartId": "2", "state": "waiting_on_database"}, + {"chartId": "3", "state": "nothing_mounted"}, + ] + ) + + assert diagnostics.ready_holders == 1 + assert diagnostics.unready_holders == 2 + assert diagnostics.semantic_success is False + + +def test_chart_holder_diagnostics_do_not_treat_zero_holders_as_success() -> None: + diagnostics = ChartHolderDiagnostics.from_holder_states([]) + + assert diagnostics.mounted_holders == 0 + assert diagnostics.semantic_success is False Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Missing test docstrings</b></div> <div id="fix"> These three new tests have no docstrings. Repo rule [12148] requires every newly added test function to include a docstring documenting purpose, scenario, and expected outcome; sibling tests in this file (e.g. `test_working_timeout_caps_report_budget`) already follow it. ([BITO.md adaptive rule 12148]) </div> </div> <small><i>Code Review Run #b4ab75</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them -- 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]
