Repository navigation
fix(dashboard): make screenshot completion truthful - #44144
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #44144 +/- ##
==========================================
- Coverage 80.65% 80.65% -0.01%
==========================================
Files 2942 2942
Lines 175691 176029 +338
Branches 40788 40840 +52
==========================================
+ Hits 141704 141971 +267
- Misses 31325 31381 +56
- Partials 2662 2677 +15
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
msyavuz
left a comment
msyavuz
left a comment
There was a problem hiding this comment.
Core fix looks right: persisting Pending before enqueue and coalescing repeat/force POSTs closes both the spinner-as-Updated case and the per-poll task storm (every poll on a cache miss enqueued a new task on master). Two asks: fix the openapi drift check, and consider splitting the report/deck.gl readiness changes into their own PR so the API state-machine fix can land and be backported alone. Note nothing here stops an Updated entry from being evicted between the POST and a later GET, so 404-after-Updated can still happen on eviction-heavy caches.
github-actions
Bot
added
the
requires:rebase
4a90704 to
3044ae8
Compare
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
github-actions
Bot
removed
the
requires:rebase
msyavuz
left a comment
msyavuz
left a comment
There was a problem hiding this comment.
Prior threads check out against the head: default readiness predicate is logic-equivalent to master and the report dwell path is unchanged, locks and the setup-failure fallback are ordered correctly, markers match the backend selectors, openapi matches the schemas. pending() still nulls a good artifact on force, but since the UI now GETs only after Updated that's no longer observable.
One recurring gap in the new code: every readiness marker only has loading | rendered, with no terminal failed value. So a map/style/tile/registry failure doesn't produce a prompt Error, it burns the full readiness budget and then errors. The four inline comments below are instances of that; a failed marker value the backend selector treats as terminal would fix all of them.
45a1d09 to
6058626
Compare
github-actions
Bot
removed
doc
217b79e to
25ec992
Compare
geido
changed the title
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
Code Review Agent Run #6d771cActionable Suggestions - 0Additional Suggestions - 5
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
EnxDev
left a comment
EnxDev
left a comment
There was a problem hiding this comment.
I traced the generation lifecycle and capture changes at 25ec992 and found one cache-expiry case worth fixing below. I reproduced it with the PR's cache helpers and a controlled SimpleCache clock. The focused pytest run couldn't start because the configured SQLite test database was unavailable, so I haven't validated the full suite locally.
25ec992 to
a55d5eb
Compare
EnxDev
left a comment
EnxDev
left a comment
There was a problem hiding this comment.
Thanks for fixing the generation-key reuse. I checked the UUID change and the expiry regression passes. This pass found one more case in the worker's cache-error handling, reproduced below. Focused validation: 204 tests passed; 10 failed because the isolated SQLite test database has no key_value table, so those lock-dependent checks remain unverified.
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #20778e
Actionable Suggestions - 1
-
superset-frontend/src/dashboard/hooks/useDownloadScreenshot.test.ts - 1
- Global URL stub never restored · Line 258-261
Additional Suggestions - 17
-
superset/dashboards/api.py - 3
-
Long blocking retry loop · Line 2209-2220The `LockAlreadyHeldException` path sleeps `SCREENSHOT_API_LOCK_RETRY_SECONDS` and loops until `lock_deadline`, which is `producer_lock_ttl` (default 30s) ahead. Concurrent requests for the same dashboard can each block a web thread for up to ~30s and issue hundreds of lock polls, risking thread-pool exhaustion under load. Verify this is acceptable.
-
Magic timeout multiplier · Line 2010-2010`task_timeout_seconds = 2 * current_app.config["THUMBNAIL_COMPUTING_CACHE_TTL"]` hardcodes the business rule that the task timeout is twice the computing TTL. A named constant would document intent and avoid silent divergence if the TTL or multiplier changes.
-
Unreachable cache-key guard · Line 2077-2086The `if cache_key is None:` branch inside `if cached_payload is not None:` is unreachable: `get_current_generation()` returns `get_from_cache_key(cache_key)` only when `cache_key` is truthy, so a non-None `cached_payload` implies `cache_key` is set. Dead defensive code adds confusion about the invariant. Remove the block.
-
-
superset-frontend/src/dashboard/hooks/useDownloadScreenshot.test.ts - 1
-
Magic retry-count coupling · Line 254-254The mock resolves only when `getCalls > 31` (line 254) while the driver loop runs exactly 31 ticks (line 268); the bare 31 in both places encodes 'initial GET + 31 retries = 32 calls' against the 120 s budget (`Math.ceil(120000 / RETRY_INTERVAL) = 40`). Bumping either the mock threshold or `task_timeout_seconds` silently breaks this tightly coupled pair — name the shared bound in a constant.
-
-
tests/unit_tests/utils/test_screenshot_utils.py - 3
-
Duplicated evaluate stub across tests · Line 568-575This `evaluate` dispatch stub (scrollWidth / `CONTENTFUL_CHART_HOLDERS_IN_CLIP_JS` / requestAnimationFrame branches) is duplicated verbatim in the sibling new test at line 647 and in pre-existing `test_repeated_blank_tiles_fail_closed_for_reports` and `test_blank_combined_image_fails_closed_after_contentful_tiles_pass`. Extract a shared helper so a dispatch-contract change updates one place.
-
Missing docstring and type hints · Line 565-565BITO.md adaptive rule 12148 requires every newly added test function to carry a docstring documenting purpose and expected behavior, and rule 12898 requires complete type annotations including `-> None` on test methods. `test_repeated_blank_tiles_fail_closed_for_api_exports` has neither, unlike documented siblings such as `test_unready_holder_state_classification_embedded_in_js`. Add a docstring and `-> None`.
-
Missing docstring and type hints · Line 644-644Same org-mandated gaps as the sibling at line 565: BITO.md adaptive rule 12148 requires a docstring and rule 12898 requires full type annotations including `-> None` on every newly added test function. `test_blank_combined_image_fails_closed_for_api_exports` has neither. Add a docstring and `-> None`.
-
-
superset/utils/screenshots.py - 4
-
Duplicated strict-write branching · Line 515-518The strict/lenient write branch (`if self.require_complete_capture: store_cache_payload(...) else: cache.set(...)`) is duplicated at the Computing write (lines 515-518) and the terminal write (lines 583-601). A shared persist helper would keep both sites in sync as capture modes or backends evolve.
-
Inconsistent status check style · Line 587-587This diff introduces `is_updated()` and uses it in `mark_cache_error_if_incomplete` (line 443), but the fallback path in `compute_and_cache` still compares `cache_payload.status == StatusValues.UPDATED` directly. Using the helper here keeps terminal-state checks consistent in one place.
-
Too many arguments · Line 466-466The `compute_and_cache` method has too many arguments (6 > 5). Consider grouping related parameters into a configuration object or using keyword-only arguments to reduce the count.
-
Too many branches · Line 466-466The `compute_and_cache` method has too many branches (16 > 12). Consider extracting sub-logic into separate methods to improve readability.
-
-
tests/unit_tests/utils/test_screenshot_cache_fix.py - 2
-
Missing test docstrings · Line 360-388The two new test methods lack docstrings, while every sibling test in this class (e.g. `test_no_trigger_on_fresh_error`) documents its scenario and expected outcome. Per BITO.md adaptive rule 12148, add a docstring to each new test method.
-
Misplaced test class · Line 360-388These two tests exercise `should_enqueue_task`, but they are placed inside `TestShouldTriggerTask` (whose docstring says it tests `should_trigger_task`). Grouping tests by the method under test keeps the suite navigable; consider a dedicated `TestShouldEnqueueTask` class.
-
-
tests/unit_tests/utils/screenshot_test.py - 4
-
Missing test docstrings · Line 159-200Org rule 12148 requires every newly added test function to include a docstring documenting purpose and expected behavior. The three new `mark_cache_error_if_incomplete` tests have none, while older tests here (e.g. `test_get_cache_key`) carry them. One-line docstrings keep the suite consistent with the standard.
-
Unannotated mock variables · Line 162-162Org rules 12787/13153 require explicit annotations on mock and local variables in test files, even when inferable. `cache = MagicMock()` (lines 162, 178, 193) and `stored_payload = cache.set.call_args.args[1]` (line 185) are unannotated; `cache: MagicMock` and `stored_payload: ScreenshotCachePayloadType` satisfy the rules.
-
Missing test docstrings · Line 229-248Org rule 12148 requires every newly added test function to include a docstring. The two new `get_from_cache_key` failure-path tests (`test_cache_read_failure_preserves_legacy_exception_behavior`, `test_cache_read_failure_can_be_distinguished_by_api`) have none; one-line docstrings documenting the default-vs-`raise_on_error` contract keep them consistent with the standard.
-
Unannotated mock variables · Line 230-230Org rule 12787 requires explicit type annotations on mock variables. `cache = MagicMock()` (lines 232 and 243) is unannotated; `cache: MagicMock = MagicMock()` satisfies the rule and matches the typed-parameter style these new tests already use for `monkeypatch`.
-
Review Details
-
Files reviewed - 14 · Commit Range:
18dadf2..fe43f35- superset-frontend/src/dashboard/hooks/useDownloadScreenshot.test.ts
- superset-frontend/src/dashboard/hooks/useDownloadScreenshot.ts
- superset/dashboards/api.py
- superset/dashboards/schemas.py
- superset/tasks/thumbnails.py
- superset/utils/screenshot_utils.py
- superset/utils/screenshots.py
- superset/utils/webdriver.py
- tests/integration_tests/dashboards/api_tests.py
- tests/unit_tests/tasks/test_thumbnails.py
- tests/unit_tests/utils/screenshot_test.py
- tests/unit_tests/utils/test_screenshot_cache_fix.py
- tests/unit_tests/utils/test_screenshot_utils.py
- tests/unit_tests/utils/webdriver_test.py
-
Files skipped - 1
- docs/static/resources/openapi.json - Reason: Filter setting
-
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers an incremental AI Review. -
/review full- Manually triggers a full AI Review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
fe43f35 to
242940d
Compare
EnxDev
left a comment
EnxDev
left a comment
There was a problem hiding this comment.
The longer contention wait helps callers join a slow producer. I found one remaining lease-expiry race in pointer publication, detailed inline. I reproduced it with the actual handler body and cache helpers, using a controlled clock and a lock double that models TTL expiry and ownership-safe release.
Validation: all 221 focused backend tests passed with the missing key_value table supplied in isolated SQLite, and all 7 download-hook tests passed on Node 24. API integration tests and live browser exports remain unverified locally.
Code Review Agent Run #d97c30Actionable Suggestions - 0Additional Suggestions - 5
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
242940d to
a843e1e
Compare
Code Review Agent Run #447698Actionable Suggestions - 0Additional Suggestions - 2
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
EnxDev
left a comment
EnxDev
left a comment
There was a problem hiding this comment.
I rechecked the latest revision and found one remaining lease-expiry case during cache I/O, detailed inline with a controlled reproduction.
Validation: all 222 focused backend screenshot tests and all 7 download-hook tests passed locally. API integration tests and live browser exports weren't run.
a843e1e to
c4d477a
Compare
There was a problem hiding this comment.
Code Review Agent Run #83ad27
Actionable Suggestions - 3
-
superset/extensions/metastore_cache.py - 1
- CAS create skips audit fields · Line 167-173
-
tests/unit_tests/utils/screenshot_test.py - 1
- Private attribute coupling · Line 120-120
-
superset-frontend/src/dashboard/hooks/useDownloadScreenshot.ts - 1
- Duplicated retry math · Line 107-109
Additional Suggestions - 9
-
superset/utils/screenshots.py - 1
-
Pointer cache on base class · Line 306-306`api_pointer_cache` is consumed only by `DashboardScreenshot`'s pointer methods (`_get_api_pointer_cache`, `set_current_api_generation_cache_key`), yet it is declared on shared base `BaseScreenshot` next to `cache`/`cache_scope`, which both chart and dashboard flows use. Declaring it on `DashboardScreenshot` keeps this dashboard-only test seam beside its only consumers.
-
-
superset-frontend/src/dashboard/hooks/useDownloadScreenshot.test.ts - 1
-
Weak test assertion · Line 286-286This test drives 31 retries (93s), below both the server budget (120s -> 40 retries) and the default (360s -> 120 retries), so it passes even if `task_timeout_seconds` is ignored. It doesn't verify the server budget overrides the default. Drive >120 retries or assert exhaustion at a server-budget count smaller than default to actually test the feature.
-
-
superset/extensions/metastore_cache.py - 3
-
Redundant duplicate except clause · Line 181-187`except (SQLAlchemyError, KeyValueCreateFailedError)` and the following `except Exception` have identical bodies (rollback + raise), so the first clause is behaviorally redundant — codec and DAO failures alike fall through to the broad handler. Collapse to a single `except Exception:` handler to remove the duplicated rollback/raise block.
-
Disallow dynamic type in value · Line 117-117The `value` parameter is typed as `Any`, which is disallowed by `ANN401`. Consider using a more specific type or `object`. Note that similar issues exist for `expected` (line 118) and the return type of `get` (line 189).
-
Disallow dynamic type in expected · Line 118-118The `expected` parameter is typed as `Any`, which is disallowed by `ANN401`. This is similar to the issue on line 117; apply the same fix (e.g., use `object`).
-
-
tests/unit_tests/utils/test_screenshot_cache_fix.py - 1
-
Flaky timing-sensitive test · Line 399-406`fresh_payload` is built with `timestamp=now` (line 389) then asserted `should_enqueue_task(force=True, force_retry_after_seconds=1) is False`. `is_in_progress_stale(1)` returns `elapsed >= 1`, so if >1s elapses between construction and the assertion (slow/loaded CI), the result flips to True and the test fails. Widen the window or mock `datetime.now()`.
-
-
tests/unit_tests/utils/screenshot_test.py - 1
-
Test depends on ambient config · Line 110-124Unlike sibling driver tests (`TestBaseScreenshotDriverFallback`, `TestScreenshotSubclassesDriverBehavior`), this test never patches `PLAYWRIGHT_AVAILABLE` or the feature flag, so `BaseScreenshot.driver()` takes the fallback path and `isinstance(driver, WebDriverPlaywright)` depends on ambient config. Under the default fallback the assertion can fail; pin both to make the Playwright branch deterministic.
-
-
superset/dashboards/api.py - 2
-
Excessive return statements in function · Line 1913-1913The function `cache_dashboard_screenshot` has 22 return statements, exceeding the limit of 6. Consider refactoring into smaller helper functions to reduce complexity.
-
Disallowed dynamic type in kwargs · Line 1914-1914The `**kwargs` parameter uses `typing.Any`, which is disallowed by ANN401. Consider using a more specific type or adding a type ignore comment if the dynamic nature is intentional.
-
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
tests/unit_tests/utils/test_screenshot_utils.py - 1
- duplicated evaluate mock · Line 565-598
Review Details
-
Files reviewed - 16 · Commit Range:
e96d9e1..c4d477a- superset-frontend/src/dashboard/hooks/useDownloadScreenshot.test.ts
- superset-frontend/src/dashboard/hooks/useDownloadScreenshot.ts
- superset/dashboards/api.py
- superset/dashboards/schemas.py
- superset/extensions/metastore_cache.py
- superset/tasks/thumbnails.py
- superset/utils/screenshot_utils.py
- superset/utils/screenshots.py
- superset/utils/webdriver.py
- tests/integration_tests/dashboards/api_tests.py
- tests/integration_tests/extensions/metastore_cache_test.py
- tests/unit_tests/tasks/test_thumbnails.py
- tests/unit_tests/utils/screenshot_test.py
- tests/unit_tests/utils/test_screenshot_cache_fix.py
- tests/unit_tests/utils/test_screenshot_utils.py
- tests/unit_tests/utils/webdriver_test.py
-
Files skipped - 1
- docs/static/resources/openapi.json - Reason: Filter setting
-
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers an incremental AI Review. -
/review full- Manually triggers a full AI Review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| KeyValueDAO.create_entry( | ||
| resource=RESOURCE, | ||
| key=cache_key, | ||
| value=value, | ||
| codec=self.codec, | ||
| expires_on=expires_on, | ||
| ) |
There was a problem hiding this comment.
KeyValueDAO.create_entry sets only created_on/created_by_fk — never changed_on/changed_by_fk — so rows created by the expected is None branch get NULL audit fields, while the update branches (lines 137-142) populate both. Assign entry.changed_on = now and entry.changed_by_fk = get_user_id() on the returned entry before db.session.commit().
Code Review Run #83ad27
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| driver = screenshot.driver() | ||
|
|
||
| assert isinstance(driver, WebDriverPlaywright) | ||
| assert driver._require_complete_capture is True # pylint: disable=protected-access |
There was a problem hiding this comment.
Reaching into driver._require_complete_capture couples the test to a private attribute of WebDriverPlaywright (webdriver.py:198). A rename or refactor of the private field breaks this test even when behavior is unchanged; asserting the public contract (e.g. via patched constructor kwargs) is more robust.
Code Review Run #83ad27
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| let maxRetries = Math.ceil( | ||
| (DEFAULT_SCREENSHOT_TASK_TIMEOUT_SECONDS * 1000) / RETRY_INTERVAL, | ||
| ); |
There was a problem hiding this comment.
The Math.ceil((timeoutSeconds * 1000) / RETRY_INTERVAL) computation is duplicated at lines 107-109 (default) and 231-233 (backend task_timeout_seconds). If the formula changes (e.g. adding a buffer or rounding policy), the two can diverge. Extract a small helper computeMaxRetries(timeoutSeconds) and call it from both sites.
Code Review Run #83ad27
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
(cherry picked from commit fe2ed7b)
SUMMARY
Make API/UI dashboard screenshot completion truthful:
Updatedis persisted only with a successfully captured artifact after the selected dashboard state reaches a stable terminal render state.Pendinggeneration before broker I/O, preserve prior artifact URLs, and coalesce concurrent forced requests.Errorfor incomplete captures and setup/cache failures without overwriting output completed by another worker.202producer /200observer convention.The signal covers Superset's chart-holder lifecycle plus the existing ECharts/AG Grid paint signals and blank-image validation. Built-in or third-party plugins that finish asynchronously in canvas/WebGL without one of those signals, including map visualizations, retain their existing readiness behavior; defining a plugin-wide completion contract is separate work.
Other deliberate scope boundaries:
master; this PR only consumes terminal taskErrormetadata returned with the screenshot endpoint's 404 response.THUMBNAIL_COMPUTING_CACHE_TTLretains its documented requirement to exceed the Celery task's 300-second soft limit (default: 360 seconds). Validation of invalid custom configurations is separate from this fix.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
On exact
master, a below-fold chart backed by a 45-second query and a 30-second screenshot wait was cached asUpdated; GET returned an 11,337-byte PNG containing a loading spinner. With this change, the same fixture transitionsPending→Computing→Errorand GET returns 404 in both standard and tiled modes.A browser UI export from a non-first top-level tab completed as
Updatedand downloaded the selected chart.TESTING INSTRUCTIONS
Automated and local verification on final commit
c4d477abce:pylintexecutable, and rated the changed production files 10/10.Errorplus artifact GET 404 for a chart exceedingSCREENSHOT_LOAD_WAIT.Updatedand its PNG remained downloadable. A forced request after the coalescing grace created a fresh generation, while non-forced callers retained the normal computing TTL.Manual verification:
THUMBNAILSandENABLE_DASHBOARD_SCREENSHOT_ENDPOINTS, configure the screenshot cache/Celery, and start Superset plus a worker./api/v1/dashboard/<id>/cache_dashboard_screenshot/and recordcache_key,dashboard_url, andimage_url.dashboard_url, then poll the same POST endpoint with{"permalinkKey": "<key>"}untiltask_statusis terminal.Updatedand the returned PNG/PDF URL is HTTP 200.SCREENSHOT_LOAD_WAIT; confirm terminalErrorand artifact GET 404.Pendingwhile the prior generation URL remains downloadable.ADDITIONAL INFORMATION
THUMBNAILS,ENABLE_DASHBOARD_SCREENSHOT_ENDPOINTSThe atomic request pointer reuses the existing metadata key-value table and requires no schema migration.