aminghadersohi commented on code in PR #42273:
URL: https://github.com/apache/superset/pull/42273#discussion_r3692921893
##########
superset/utils/webdriver.py:
##########
@@ -563,18 +563,23 @@ def get_screenshot( # pylint: disable=too-many-locals,
too-many-statements # n
log_context=log_context,
)
if not img:
+ # _get_screenshot() has no wait/readiness logic at
+ # all, so falling back to it here would risk
+ # silently delivering a screenshot of spinners or
+ # a blank dashboard. Fail the capture loudly
+ # (report error, thumbnail cache ERROR) instead of
+ # guessing at a "safer" fallback.
Review Comment:
This branch has now flipped twice. #41080 removed the fallback for exactly
the reason the new comment gives (blank capture silently reaching report
recipients), and #41097 re-added it eleven days later while fixing the
`b""`/COMPUTING retry loop — its description presents the fallback as the
intended behavior and doesn't mention #41080.
The comment explains why there is no fallback, but not why the reason it
came back no longer applies. Someone arriving from #41097 has nothing telling
them not to restore it. Worth pinning the history and the reason the retry loop
stays closed: `compute_and_cache()` catches the exception and calls
`cache_payload.error()`, so the payload leaves `COMPUTING` exactly as it did
when the fallback produced a falsy image.
```suggestion
# _get_screenshot() has no wait/readiness logic
at
# all, so falling back to it here would risk
# silently delivering a screenshot of spinners or
# a blank dashboard. Fail the capture loudly
# (report error, thumbnail cache ERROR) instead
of
# guessing at a "safer" fallback.
# History: removed in #41080, re-added in #41097
to
# stop a COMPUTING-status retry loop; raising
keeps
# that loop closed via compute_and_cache()'s
ERROR
# transition. Re-read both before restoring it.
```
--
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]
