korbit-ai[bot] commented on code in PR #32196:
URL: https://github.com/apache/superset/pull/32196#discussion_r1968636721
##########
superset/views/core.py:
##########
@@ -846,15 +846,22 @@ def dashboard_permalink(
return redirect("/dashboard/list/")
if not value:
return json_error_response(_("permalink state not found"),
status=404)
+
dashboard_id, state = value["dashboardId"], value.get("state", {})
url = f"/superset/dashboard/{dashboard_id}?permalink_key={key}"
if url_params := state.get("urlParams"):
- params = parse.urlencode(url_params)
- url = f"{url}&{params}"
+ for param_key, param_val in url_params:
+ if param_key == "native_filters":
+ # native_filters doesnt need to be encoded here
+ url = f"{url}&native_filters={param_val}"
+ else:
+ params = parse.urlencode([param_key, param_val])
+ url = f"{url}&{params}"
Review Comment:
### Incorrect URL Parameters Iteration <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The code iterates over url_params which is assumed to be a sequence of
tuples, but url_params is likely a dictionary from the state object. This will
cause a ValueError during iteration.
###### Why this matters
This will cause the native filters functionality to fail when trying to
process dashboard permalinks, preventing users from sharing dashboards with
specific filter configurations.
###### Suggested change ∙ *Feature Preview*
```python
for param_key, param_val in url_params.items():
if param_key == "native_filters":
# native_filters doesnt need to be encoded here
url = f"{url}&native_filters={param_val}"
else:
params = parse.urlencode([(param_key, param_val)])
url = f"{url}&{params}"
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b1f5f8e3-60ce-4b6a-8160-cdcec5dce6fc?suggestedFixEnabled=true)
💬 Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:153e18a9-5dfb-4e9a-a75c-6a592e6fd667 -->
##########
superset/commands/report/execute.py:
##########
@@ -228,16 +228,30 @@ def get_dashboard_urls(
Retrieve the URL for the dashboard tabs, or return the dashboard URL
if no tabs are available.
""" # noqa: E501
force = "true" if self._report_schedule.force_screenshot else "false"
+
if (
dashboard_state := self._report_schedule.extra.get("dashboard")
) and feature_flag_manager.is_feature_enabled("ALERT_REPORT_TABS"):
if anchor := dashboard_state.get("anchor"):
try:
anchor_list: list[str] = json.loads(anchor)
- return self._get_tabs_urls(anchor_list,
user_friendly=user_friendly)
+ urls = self._get_tabs_urls(anchor_list,
user_friendly=user_friendly)
+ return urls
except json.JSONDecodeError:
logger.debug("Anchor value is not a list, Fall back to
single tab")
- return [self._get_tab_url(dashboard_state)]
+
+ native_filter_params =
self._report_schedule.get_native_filters_params()
Review Comment:
### Missing Native Filters Parameters Method <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The code assumes the existence of get_native_filters_params() method on the
report_schedule object, but this method is not defined in the provided code.
###### Why this matters
This will result in an AttributeError when trying to execute reports with
native filters, causing the report generation to fail.
###### Suggested change ∙ *Feature Preview*
Define the get_native_filters_params method in the ReportSchedule class:
```python
def get_native_filters_params(self) -> str:
"""Get native filters parameters from the report schedule extra field"""
if filters := self.extra.get('filters'):
return json.dumps(filters)
return '{}'
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/46620ad2-e4ab-4e82-b1cb-e063acc41a2a?suggestedFixEnabled=true)
💬 Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:a3e3f508-d66b-45f5-a939-14efe44b66ae -->
##########
superset/commands/report/execute.py:
##########
@@ -320,6 +335,7 @@ def _get_screenshots(self) -> list[bytes]:
]
else:
urls = self.get_dashboard_urls()
+ print("urls", urls)
Review Comment:
### Sensitive Information Exposure in Print Statement <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Sensitive information (URLs that may contain authentication tokens or
internal paths) is being logged using print() statements.
###### Why this matters
Logging sensitive URLs could expose internal system information or
authentication tokens to unauthorized users through log files or console output.
###### Suggested change ∙ *Feature Preview*
Remove the print statement or replace with appropriate debug logging:
```python
logger.debug("Processing dashboard URLs") # Log without exposing URLs
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e40c793c-78cf-411f-9aab-ec3dc8f035ca?suggestedFixEnabled=true)
💬 Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:8114f11c-5a54-4254-9bad-0b690551d9ea -->
##########
superset-frontend/src/features/alerts/AlertReportModal.tsx:
##########
@@ -1111,6 +1168,75 @@ const AlertReportModal:
FunctionComponent<AlertReportModalProps> = ({
setForceScreenshot(event.target.checked);
};
+ const onChangeDashboardFilter = (idx: number, nativeFilterId: string) => {
+ console.log('dashboardFilter', nativeFilterId);
+
+ // set dashboardFilter
+ const anchor = currentAlert?.extra?.dashboard?.anchor;
+ const inScopeFilters = tabNativeFilters[anchor];
+ const filter = inScopeFilters.filter(
+ (f: any) => f.id === nativeFilterId,
+ )[0];
+
+ const { datasetId } = filter.targets[0];
+ const columnName = filter.targets[0].column.name;
+ const columnLabel = nativeFilterOptions.filter(
+ filter => filter.value === nativeFilterId,
+ )[0].label;
+ const dashboardId = currentAlert?.dashboard?.value;
+
+ // Get values tied to the selected filter
+ const filterValues = {
+ formData: {
+ datasource: `${datasetId}__table`,
+ groupby: [columnName],
+ metrics: ['count'],
+ row_limit: 1000,
+ showSearch: true,
+ viz_type: 'filter_select',
+ type: 'NATIVE_FILTER',
+ dashboardId,
+ },
+ force: false,
+ ownState: {},
+ };
+
+ getChartDataRequest(filterValues).then(response => {
+ const newFilterValues = response.json.result[0].data.map((item: any) =>
({
+ value: item[columnName],
+ label: item[columnName],
+ }));
+
+ setNativeFilterData(
+ nativeFilterData.map((filter, index) =>
+ index === idx
+ ? {
+ ...filter,
+ nativeFilterId,
+ columnLabel,
+ columnName,
+ optionFilterValues: newFilterValues,
+ filterValues: [], // reset filter values on filter change
+ }
+ : filter,
+ ),
+ );
+ });
+ };
+
+ const onChangeDashboardFilterValue = (
+ idx: number,
+ filterValues: string[],
+ ) => {
+ // todo(hughhh): refactor to handle multiple native filters
+ setNativeFilterData(
+ nativeFilterData.map((filter, index) =>
+ index === idx ? { ...filter, filterValues } : filter,
+ ),
+ );
Review Comment:
### Inefficient Array State Updates <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
State updates using array mapping operations inside setNativeFilterData are
potentially inefficient for large datasets, as they create new array copies on
each update.
###### Why this matters
For large arrays, this pattern causes unnecessary memory allocation and GC
pressure due to creating intermediate arrays on each state update.
###### Suggested change ∙ *Feature Preview*
Use a more efficient update pattern that directly modifies only the changed
element:
```typescript
setNativeFilterData(prevData => {
const newData = [...prevData];
newData[idx] = { ...newData[idx], filterValues };
return newData;
});
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/60b60864-56e1-43a0-87f9-d76539291e9e?suggestedFixEnabled=true)
💬 Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:442ed6d6-dc61-413f-956e-eb5b2495e1b4 -->
--
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]
