feat: Add Dashboard Filter Support for Alert Reports - #32196
Conversation
dosubot
Bot
added
the
alert-reports
4f78062 to
6db36be
Compare
|
I was unable to post the issues I found. This could be because a force push or squash has changed the commit history since I scanned this pull request. You can get another review by commenting |
72774cd to
d6adc97
Compare
d6adc97 to
47fc7de
Compare
heidijosweeenz1997
left a comment
heidijosweeenz1997
left a comment
There was a problem hiding this comment.
Withdraw in to my commonwealth bank
8574ad9 to
a3c7b96
Compare
a3c7b96 to
bf31b9d
Compare
|
@sadpandajoe Processing your ephemeral environment request here. Action: up. More information on how to use or configure ephemeral environments |
|
@sadpandajoe Ephemeral environment spinning up at http://44.245.216.87:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup. |
…t when they click save and ignore it
![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 #b010e8
Actionable Suggestions - 23
-
tests/integration_tests/reports/commands/execute_dashboard_report_tests.py - 2
- Dashboard state access broken by structure change · Line 52-55
- Broken dashboard state access pattern · Line 95-98
-
superset-frontend/src/features/alerts/AlertReportModal.tsx - 14
- Invalid CSS display property value · Line 370-370
- Invalid CSS flex property value · Line 374-374
- Invalid CSS padding template literal · Line 375-375
- Missing error handling for API response · Line 681-681
- Missing array bounds checking · Line 722-722
- State mutation violates React immutability · Line 1274-1278
- Unsafe array access without bounds checking · Line 1404-1404
- Unsafe array access in columnLabel assignment · Line 1431-1433
- Race condition in async filter handling · Line 1484-1499
- Stale closure causing incorrect state updates · Line 1626-1633
- Stale closure causing incorrect state updates · Line 1639-1646
- Duplicate error messages in validation · Line 1725-1732
- Infinite re-render risk with array dependency · Line 1953-1953
- React state mutation violation · Line 2392-2397
-
superset/reports/models.py - 4
- KeyError on missing filter dictionary keys · Line 197-200
- IndexError on empty values list access · Line 220-221
- IndexError on timegrain filter values access · Line 230-230
- IndexError on timecolumn filter values access · Line 244-244
-
superset/commands/report/execute.py - 2
- Type mismatch in urlParams structure · Line 324-326
- Type mismatch in urlParams structure · Line 325-325
-
tests/unit_tests/reports/model_test.py - 1
- Invalid parameter type in test · Line 82-82
Additional Suggestions - 4
-
superset-frontend/src/features/alerts/AlertReportModal.tsx - 4
-
Incorrect CSS property for inline element · Line 110-110The CSS property `align-content: center` is inappropriate for a span element and won't achieve vertical centering. Use `vertical-align: middle` instead to properly center the divider text relative to adjacent InputNumber components.
Code suggestion
@@ -110,1 +110,1 @@ - align-content: center; + vertical-align: middle;
-
Unused import Button from components · Line 48-48The `Button` component is imported but never used in the code. Consider removing this unused import to improve code cleanliness.
Code suggestion
@@ -46,7 +46,6 @@ import { AsyncSelect, - Button, Checkbox, Collapse, CollapseLabelInModal, -
Unused import _isColumnsSortingCoupledToGroup · Line 90-90The `_isColumnsSortingCoupledToGroup` import is unused. Remove this import to keep the codebase clean and reduce bundle size.
Code suggestion
@@ -87,7 +87,6 @@ import { useSelector } from 'react-redux'; import { UserWithPermissionsAndRoles } from 'src/types/bootstrapTypes'; import { getChartDataRequest } from 'src/components/Chart/chartAction'; -import { _isColumnsSortingCoupledToGroup } from 'ag-grid-community'; import DateFilterControl from 'src/explore/components/controls/DateFilterControl'; import { Icons } from '@superset-ui/core/components/Icons'; import { StandardModal, ModalFormField } from 'src/components/Modal'; -
Unused parameter in catch block · Line 1108-1108The catch block parameter `e` is defined but never used. Either use the error parameter or remove it to follow best practices.
Code suggestion
@@ -1108,7 +1108,7 @@ }) - .catch(e => { + .catch(() => { addDangerToast(t('There was an error retrieving dashboard tabs.')); }); }
-
Review Details
-
Files reviewed - 14 · Commit Range:
38bb661..de6df8d- superset-frontend/packages/superset-ui-core/src/utils/featureFlags.ts
- superset-frontend/src/features/alerts/AlertReportModal.test.tsx
- superset-frontend/src/features/alerts/AlertReportModal.tsx
- superset-frontend/src/features/alerts/types.ts
- superset/commands/report/execute.py
- superset/config.py
- superset/daos/dashboard.py
- superset/dashboards/api.py
- superset/reports/models.py
- superset/views/core.py
- tests/integration_tests/dashboards/api_tests.py
- tests/integration_tests/reports/commands/execute_dashboard_report_tests.py
- tests/integration_tests/reports/commands_tests.py
- tests/unit_tests/reports/model_test.py
-
Files skipped - 0
-
Tools
- Eslint (Linter) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- 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 Default Agent You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| extra={ | ||
| "activeTabs": ["TAB-L1B", "TAB-L2BB"], | ||
| "urlParams": [["native_filters", "()"]], | ||
| }, |
There was a problem hiding this comment.
The extra parameter structure change breaks dashboard state access. The create_dashboard_report function wraps the extra parameter in a dashboard key (line 199 in utils.py), but the test later accesses report_schedule.extra.get("dashboard", {}) (line 62). With the new flat structure, this will return an empty dict instead of the expected dashboard state, causing the CreateDashboardPermalinkCommand to receive incorrect parameters.
Code suggestion
Check the AI-generated fix before applying
| extra={ | |
| "activeTabs": ["TAB-L1B", "TAB-L2BB"], | |
| "urlParams": [["native_filters", "()"]], | |
| }, | |
| extra={"dashboard": { | |
| "activeTabs": ["TAB-L1B", "TAB-L2BB"], | |
| "urlParams": [["native_filters", "()"]], | |
| }}, |
Code Review Run #b010e8
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| extra={ | ||
| "active_tabs": ["TAB-L1B", "TAB-L2BB"], | ||
| "urlParams": [["native_filters", "()"]], | ||
| }, |
There was a problem hiding this comment.
The flattened extra parameter structure breaks dashboard state access. The create_dashboard_report function wraps the extra parameter under a dashboard key, but the test code expects to access it via report_schedule.extra.get('dashboard', {}). With the new structure, dashboard_state will be an empty dictionary, causing CreateDashboardPermalinkCommand to fail with incorrect parameters. Revert to the nested structure: extra={'dashboard': {'active_tabs': [...], 'urlParams': [...]}} to maintain compatibility.
Code suggestion
Check the AI-generated fix before applying
| extra={ | |
| "active_tabs": ["TAB-L1B", "TAB-L2BB"], | |
| "urlParams": [["native_filters", "()"]], | |
| }, | |
| extra={"dashboard": {"active_tabs": ["TAB-L1B", "TAB-L2BB"], "urlParams": [["native_filters", "()"]]}}, |
Code Review Run #b010e8
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
|
|
||
| .filters-trashcan { | ||
| width: ${theme.sizeUnit * 10}px; | ||
| display: 'flex'; |
There was a problem hiding this comment.
Invalid CSS display property value. The display: 'flex' has quotes around 'flex', making it a literal string instead of a CSS value. This will break the flexbox layout. Remove the quotes to fix: display: flex;
Code suggestion
Check the AI-generated fix before applying
| display: 'flex'; | |
| display: flex; |
Code Review Run #b010e8
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| color: ${theme.colorIcon}; | ||
| } | ||
| .filters-add-container { | ||
| flex: '.25'; |
There was a problem hiding this comment.
Invalid CSS flex property value. The flex: '.25' has quotes around '.25', making it a literal string instead of a numeric CSS value. This will break the flex layout. Remove the quotes to fix: flex: 0.25;
Code suggestion
Check the AI-generated fix before applying
| flex: '.25'; | |
| flex: 0.25; |
Code Review Run #b010e8
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| } | ||
| .filters-add-container { | ||
| flex: '.25'; | ||
| padding: '${theme.sizeUnit * 3} 0'; |
There was a problem hiding this comment.
Invalid CSS padding property value. The padding: '${theme.sizeUnit * 3} 0' has quotes around the template literal, preventing theme variable interpolation. This will break the padding calculation. Remove the quotes to fix: padding: ${theme.sizeUnit * 3}px 0;
Code suggestion
Check the AI-generated fix before applying
| padding: '${theme.sizeUnit * 3} 0'; | |
| padding: ${theme.sizeUnit * 3}px 0; |
Code Review Run #b010e8
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| native_filter_id or "": { | ||
| "id": native_filter_id or "", | ||
| "extraFormData": { | ||
| "time_grain_sqla": values[0], # grain |
There was a problem hiding this comment.
IndexError risk: values[0] accessed without bounds checking in filter_timegrain handler. This will crash when values list is empty. Add check: values[0] if values else None.
Code suggestion
Check the AI-generated fix before applying
| "time_grain_sqla": values[0], # grain | |
| "time_grain_sqla": values[0] if values else None, # grain |
Code Review Run #b010e8
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| return { | ||
| native_filter_id or "": { | ||
| "extraFormData": { | ||
| "granularity_sqla": values[0] # column_name |
There was a problem hiding this comment.
IndexError risk: values[0] accessed without bounds checking in filter_timecolumn handler. This will crash when values list is empty. Add check: values[0] if values else None.
Code suggestion
Check the AI-generated fix before applying
| "granularity_sqla": values[0] # column_name | |
| "granularity_sqla": values[0] if values else None # column_name |
Code Review Run #b010e8
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| "urlParams": [ | ||
| ["native_filters", native_filter_params] # type: ignore | ||
| ], |
There was a problem hiding this comment.
Type mismatch in urlParams: The DashboardPermalinkState type expects urlParams to be Optional[list[tuple[str, str]]], but the code provides list[list[str]] by using ["native_filters", native_filter_params]. This should be ("native_filters", native_filter_params) to create a tuple instead of a list.
Code suggestion
Check the AI-generated fix before applying
| "urlParams": [ | |
| ["native_filters", native_filter_params] # type: ignore | |
| ], | |
| "urlParams": [ | |
| ("native_filters", native_filter_params) # type: ignore | |
| ], |
Code Review Run #b010e8
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| "activeTabs": None, | ||
| "urlParams": None, | ||
| "urlParams": [ | ||
| ["native_filters", native_filter_params] # type: ignore |
There was a problem hiding this comment.
Type mismatch in urlParams: DashboardPermalinkState expects list[tuple[str, str]] but the code provides list[list[str]]. Change ["native_filters", native_filter_params] to ("native_filters", native_filter_params) to match the expected tuple type.
Code suggestion
Check the AI-generated fix before applying
| ["native_filters", native_filter_params] # type: ignore | |
| ("native_filters", native_filter_params) # type: ignore |
Code Review Run #b010e8
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| native_filter_id = "filter_id" | ||
| column_name = "column_name" | ||
| filter_type = "filter_select" | ||
| values = None |
There was a problem hiding this comment.
The test passes None as the values parameter to _generate_native_filter, but the method signature expects list[Optional[str]]. This will cause a TypeError when the method tries to access values[0] for time filters or use values or [] operations. Change values = None to values = [] to match the expected parameter type.
Code suggestion
Check the AI-generated fix before applying
| values = None | |
| values = [] |
Code Review Run #b010e8
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Code Review Agent Run #6f609aActionable 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 |
geido
left a comment
geido
left a comment
There was a problem hiding this comment.
Approving this as a good major step while we working on refining details / fixing minor issues.
6 participants
Summary
This PR introduces a new feature that allows users to configure dashboard filters when creating alert reports. The feature is controlled by the
ALERT_REPORTS_FILTERfeature flag and enables users to specify which native filters should be applied when generating dashboard reports.Key Features
filter_time)filter_range) with min/max valuesfilter_select) with multiple value selectionfilter_timecolumn)filter_timegrain)Technical Implementation
Frontend Changes
TreeSelectfor filter selectionBackend Changes
Feature Flag
Truein development environmentsUser Experience
Testing
Configuration
The feature is enabled by default but can be controlled via the
ALERT_REPORTS_FILTERfeature flag insuperset_config.py:Breaking Changes
None - this is a purely additive feature that doesn't affect existing functionality.
Documentation
ALERT_REPORTS_FILTERflagThis enhancement significantly improves the flexibility of alert reports by allowing users to generate