fix(reports): preserve urlParams in multi-tab report fan-out#39884
fix(reports): preserve urlParams in multi-tab report fan-out#39884
Conversation
Code Review Agent Run #00d20eActionable Suggestions - 0Review 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 |
dosubot
Bot
added
alert-reports
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #39884 +/- ##
==========================================
- Coverage 63.87% 63.87% -0.01%
==========================================
Files 2582 2582
Lines 136413 136420 +7
Branches 31453 31453
==========================================
+ Hits 87136 87140 +4
- Misses 47764 47767 +3
Partials 1513 1513
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
EnxDev
force-pushed
the
enxdev/fix/alerts-reports-filter-time-range-multitab-timeout
branch
from
6d049b3 to
25a654b
Compare
`_get_tabs_urls` and the `ALERT_REPORT_TABS`-disabled fall-through in
`get_dashboard_urls` were both building permalinks with
`urlParams=[["native_filters", <rison>]]` only, silently discarding
everything else `extra.dashboard.urlParams` carried — notably
`standalone=true`, which the single-tab branch already preserves
(see `get_dashboard_urls` lines 290-306).
The list-anchor shape that triggers the multi-tab path is reachable
from the standard Reports UI: when a dashboard has at least 2 tabs,
the AlertReportModal injects an "All Tabs" option whose value is
`JSON.stringify(allTabsWithOrder)` (see `AlertReportModal.tsx`
~L1094-1099). Picking it makes the backend fan out per-tab screenshots,
each missing `standalone=true`, which causes Playwright to time out at
`Locator.wait_for(".standalone")` after the 10-minute hard timeout.
Recipients get a failure email instead of the screenshots.
Apply the same merge semantics in both spots: dedup any existing
`native_filters` entry from the original `urlParams` and append the
report's. Other per-tab fields (`anchor`, `dataMask`, `activeTabs`)
keep their previous shape — this fix is scoped to the `urlParams`
discard. The `ALERT_REPORT_TABS=False` fall-through previously didn't
honor `extra.dashboard.urlParams` at all (only API-set, since the
modal is gated on the flag); now it does, matching the on-flag path.
Drive-bys (same area, kept tight):
- `DashboardPermalinkState.urlParams` annotation: `list[tuple[str, str]]`
-> `list[Sequence[str]]`. JSON-derived values arrive as
`list[list[str]]` at runtime; `Sequence` is permissive of both shapes.
- Single-tab branch and the merged fall-through use the new annotation;
obsolete `# type: ignore` comments removed.
- Silenced a pre-existing pylint `consider-using-transaction` warning
on `db.session.rollback()` in `create_log` that the pre-commit hook
was sporadically failing on; same disable comment as the surrounding
`db.session.commit()`.
Tests:
- `test_get_dashboard_urls_multitab_preserves_url_params` seeds
`urlParams` with `standalone`, `show_filters`, AND a stale
`native_filters` entry, and asserts the merge replaces the stale
entry, preserves the others in order, and targets each per-tab
permalink at its anchor.
- `test_get_dashboard_urls_flag_off_preserves_url_params` does the
same for the `ALERT_REPORT_TABS=False` fall-through.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
EnxDev
force-pushed
the
enxdev/fix/alerts-reports-filter-time-range-multitab-timeout
branch
from
25a654b to
f9c665f
Compare
Address review feedback on the flag-off urlParams preservation test: - Drop dead `force_screenshot = False` — the merge branch returns early so the attribute is never read. Mock auto-attributes are truthy by default; the matching multi-tab test doesn't set it either. - Expand the docstring with a "Reachability:" note explaining that the fall-through is reached only when `dashboard_state` is falsy OR `ALERT_REPORT_TABS=False`; the flag-on / no-anchor case lands in the single-tab merge at L290-306 (already correct since before this PR), not in this branch. No production change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Code Review Agent Run #25373aActionable Suggestions - 0Review 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 |
| ) | ||
| urls = self._get_tabs_urls( | ||
| anchor_list, | ||
| dashboard_state=dashboard_state, |
There was a problem hiding this comment.
Why is this necessary?
There was a problem hiding this comment.
Because _get_tabs_urls now needs the original dashboard_state to read its urlParams
| except StaleDataError as ex: | ||
| # Report schedule was modified or deleted by another process | ||
| db.session.rollback() | ||
| db.session.rollback() # pylint: disable=consider-using-transaction |
There was a problem hiding this comment.
it's a pre-existing pylint warning on pre-existing code in create_log, I added the disable comment because the pre-commit pylint hook failing
| base_state: DashboardPermalinkState = dashboard_state or {} | ||
| existing_params: list[Sequence[str]] = base_state.get("urlParams") or [] | ||
| merged_params: list[Sequence[str]] = [ | ||
| list(p) for p in existing_params if p[0] != "native_filters" | ||
| ] | ||
| merged_params.append(["native_filters", native_filter_params or ""]) |
There was a problem hiding this comment.
I think this is repeated a bunch now, should we extract it out?
There was a problem hiding this comment.
Yes, I'm going to create a static helper
|
The changes in superset/commands/report/execute.py involve updates to type annotations, adding imports, and modifying logic for handling dashboard URLs and native filters in report execution. Without the specific diff hunk or comment context, I can't pinpoint the exact necessity, but they appear to address parameter merging and state preservation. |
Code Review Agent Run #5a75f1Actionable Suggestions - 0Review 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 |
3 participants
SUMMARY
_get_tabs_urlsand theALERT_REPORT_TABS-disabled fall-through inget_dashboard_urlswere both building permalinks withurlParams=[["native_filters", <rison>]]only, silently discarding everything elseextra.dashboard.urlParamscarried — notablystandalone=true, which the single-tab branch already preserves(see
get_dashboard_urlslines 290–306).The list-anchor shape that triggers the multi-tab path is reachable from the standard Reports UI: when a dashboard has ≥2 tabs, the Alert/Report modal injects an "All Tabs" option whose value is
JSON.stringify(allTabsWithOrder)(seeAlertReportModal.tsx~L1094–1099). Picking it makes the backend fan out per-tab screenshots, each missingstandalone=true, which causes Playwright to time out atLocator.wait_for(".standalone")after the 10-minute hard timeout.Recipients get a failure email instead of the screenshots.
This change applies the same merge semantics in both spots: dedup any existing
native_filtersentry from the originalurlParamsand append the report's.Other per-tab fields (
anchor,dataMask,activeTabs) keep their previous shape — the fix is scoped to theurlParamsdiscard.The
ALERT_REPORT_TABS=Falsefall-through previously didn't honorextra.dashboard.urlParamsat all (onlyAPI-set, since the modal is gated on the flag); now it does, matching the on-flag path.
Drive-bys (same area, kept tight):
DashboardPermalinkState.urlParamsannotation:list[tuple[str, str]]→list[Sequence[str]]. JSON-derived values arrive aslist[list[str]]at runtime;Sequenceis permissive of both shapes.# type: ignorecomments removed.consider-using-transactionwarning ondb.session.rollback()increate_logthat the pre-commit hook was sporadically failing on; same disable comment as the surroundingdb.session.commit().BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — change is in the report screenshot URL builder; not user-visible
UI. Behavior change is observable via
report_execution_log.error_message:Failed taking a screenshot Locator.wait_for: Timeout600000ms exceeded. Call log: - waiting for locator(".standalone") to be visibleper fan-out screenshot.standalone=trueand any other params the user set; Playwright reaches the standalone view normally.TESTING INSTRUCTIONS
pytest tests/unit_tests/commands/report/execute_test.py -k url_params
ADDITIONAL INFORMATION