korbit-ai[bot] commented on code in PR #35343:
URL: https://github.com/apache/superset/pull/35343#discussion_r2410978941
##########
superset-frontend/src/dashboard/components/URLShortLinkButton/index.tsx:
##########
@@ -49,21 +49,39 @@ export default function URLShortLinkButton({
const theme = useTheme();
const [shortUrl, setShortUrl] = useState('');
const { addDangerToast } = useToasts();
- const { dataMask, activeTabs } = useSelector(
+ const { dataMask, activeTabs, chartStates, sliceEntities } = useSelector(
(state: RootState) => ({
dataMask: state.dataMask,
activeTabs: state.dashboardState.activeTabs,
+ chartStates: state.dashboardState.chartStates,
+ sliceEntities: state.sliceEntities,
}),
shallowEqual,
);
Review Comment:
### High Redux State Coupling <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The component is directly coupled to multiple Redux state properties, making
it tightly coupled to the global state structure.
###### Why this matters
High coupling to global state makes the component less portable and harder
to test. Changes to the Redux state structure would require changes to this
component.
###### Suggested change ∙ *Feature Preview*
Create a custom hook to abstract the state access and provide a cleaner
interface:
```typescript
const useUrlGenerationData = () => {
return useSelector((state: RootState) => ({
dataMask: state.dataMask,
activeTabs: state.dashboardState.activeTabs,
chartStates: state.dashboardState.chartStates,
sliceEntities: state.sliceEntities,
}), shallowEqual);
};
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/43d4b7fd-ff3b-4ae5-b4e3-73bb26413f22/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/43d4b7fd-ff3b-4ae5-b4e3-73bb26413f22?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/43d4b7fd-ff3b-4ae5-b4e3-73bb26413f22?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/43d4b7fd-ff3b-4ae5-b4e3-73bb26413f22?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/43d4b7fd-ff3b-4ae5-b4e3-73bb26413f22)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:b0dfd404-39a6-42ca-94bd-0aa3f93108a9 -->
[](b0dfd404-39a6-42ca-94bd-0aa3f93108a9)
##########
superset-frontend/src/dashboard/types/chartState.ts:
##########
@@ -0,0 +1,65 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+export interface AgGridColumnState {
+ colId: string;
+ width?: number;
+ hide?: boolean;
+ pinned?: 'left' | 'right' | null;
+ sort?: 'asc' | 'desc' | null;
+ sortIndex?: number;
+ aggFunc?: string;
+}
+
+export interface AgGridSortModel {
+ colId: string;
+ sort: 'asc' | 'desc';
+ sortIndex?: number;
+}
+
+export interface AgGridFilterModel {
+ [colId: string]: {
+ filterType: string;
+ type?: string;
+ filter?: any;
+ condition1?: any;
+ condition2?: any;
+ operator?: string;
+ };
+}
+
+export interface AgGridChartState {
+ columnState: AgGridColumnState[];
+ sortModel: AgGridSortModel[];
+ filterModel: AgGridFilterModel;
+ columnOrder?: string[];
+ pageSize?: number;
+ currentPage?: number;
+}
+
+export interface ChartState {
+ chartId: number;
+ vizType: string;
+ state: AgGridChartState;
+ lastModified?: number;
+}
Review Comment:
### Chart state tightly coupled to AgGrid <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The ChartState interface is tightly coupled to AgGrid implementation
details, violating the Interface Segregation Principle.
###### Why this matters
This coupling makes it difficult to support different chart implementations
or grid libraries in the future, as the state structure is specifically
designed for AgGrid.
###### Suggested change ∙ *Feature Preview*
Create an abstraction layer:
```typescript
export interface BaseChartState {
chartId: number;
vizType: string;
lastModified?: number;
}
export interface AgGridChartState extends BaseChartState {
state: AgGridSpecificState;
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c44bdca3-51a6-4962-9f7a-06f240246113/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c44bdca3-51a6-4962-9f7a-06f240246113?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c44bdca3-51a6-4962-9f7a-06f240246113?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c44bdca3-51a6-4962-9f7a-06f240246113?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c44bdca3-51a6-4962-9f7a-06f240246113)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:2f8d5822-242b-4569-9568-976a83e56e5f -->
[](2f8d5822-242b-4569-9568-976a83e56e5f)
##########
superset-frontend/src/dashboard/actions/dashboardState.js:
##########
@@ -747,6 +747,27 @@ export function setFullSizeChartId(chartId) {
return { type: SET_FULL_SIZE_CHART_ID, chartId };
}
+export const UPDATE_CHART_STATE = 'UPDATE_CHART_STATE';
+export function updateChartState(chartId, vizType, chartState) {
+ return {
+ type: UPDATE_CHART_STATE,
+ chartId,
+ vizType,
+ chartState,
+ lastModified: Date.now(),
+ };
+}
Review Comment:
### Missing parameter validation in updateChartState <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The updateChartState action creator does not validate its parameters,
allowing undefined or null values to be passed through to the reducer.
###### Why this matters
This could lead to corrupted state if invalid parameters are passed,
potentially causing runtime errors in components that depend on valid chart
state data or making it impossible to properly restore chart states.
###### Suggested change ∙ *Feature Preview*
Add parameter validation to ensure required fields are present:
```javascript
export function updateChartState(chartId, vizType, chartState) {
if (!chartId) {
throw new Error('chartId is required for updateChartState');
}
if (!vizType) {
throw new Error('vizType is required for updateChartState');
}
if (chartState === undefined || chartState === null) {
throw new Error('chartState is required for updateChartState');
}
return {
type: UPDATE_CHART_STATE,
chartId,
vizType,
chartState,
lastModified: Date.now(),
};
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8d51da37-b5cc-41ba-8e59-a9d6415aade2/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8d51da37-b5cc-41ba-8e59-a9d6415aade2?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8d51da37-b5cc-41ba-8e59-a9d6415aade2?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8d51da37-b5cc-41ba-8e59-a9d6415aade2?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8d51da37-b5cc-41ba-8e59-a9d6415aade2)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:ddae6aee-f914-4d9b-bacf-3334d0899df9 -->
[](ddae6aee-f914-4d9b-bacf-3334d0899df9)
##########
superset/charts/schemas.py:
##########
@@ -1018,6 +1018,27 @@ class ChartDataExtrasSchema(Schema):
},
allow_none=True,
)
+ is_ag_grid_chart = fields.Boolean(
+ metadata={
+ "description": (
+ "Flag indicating if the query is from an AG Grid table chart. "
+ "When true, enables filtering on metric columns in addition "
+ "to dimension columns."
+ )
+ },
+ allow_none=True,
+ )
Review Comment:
### Missing default value for is_ag_grid_chart boolean field <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The is_ag_grid_chart field lacks validation or default value specification,
which could lead to inconsistent behavior when the field is not explicitly set.
###### Why this matters
Without a default value, the field could be None, True, or False depending
on how it's used, potentially causing unexpected behavior in downstream logic
that depends on this flag to enable metric column filtering.
###### Suggested change ∙ *Feature Preview*
Add a default value to ensure consistent behavior:
```python
is_ag_grid_chart = fields.Boolean(
metadata={
"description": (
"Flag indicating if the query is from an AG Grid table chart. "
"When true, enables filtering on metric columns in addition "
"to dimension columns."
)
},
allow_none=True,
load_default=False,
)
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/75ce4475-d576-41be-a98f-eca939c5cf6e/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/75ce4475-d576-41be-a98f-eca939c5cf6e?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/75ce4475-d576-41be-a98f-eca939c5cf6e?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/75ce4475-d576-41be-a98f-eca939c5cf6e?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/75ce4475-d576-41be-a98f-eca939c5cf6e)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:6ca1ee0c-2231-4c7e-a188-e0d72994e1b2 -->
[](6ca1ee0c-2231-4c7e-a188-e0d72994e1b2)
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/index.tsx:
##########
@@ -114,11 +123,14 @@ const AgGridDataTable:
FunctionComponent<AgGridTableProps> = memo(
cleanedTotals,
showTotals,
width,
+ onColumnStateChange,
+ savedAgGridState,
}) => {
const gridRef = useRef<AgGridReact>(null);
Review Comment:
### Unused gridRef prop creates broken external API access <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The component creates its own gridRef but also accepts an optional gridRef
prop, creating potential conflicts and unused functionality.
###### Why this matters
The gridRef prop is accepted but never used, while the component always uses
its internal gridRef. This means external components cannot access the grid API
through the provided ref, breaking the expected functionality of the gridRef
prop.
###### Suggested change ∙ *Feature Preview*
Use the provided gridRef prop when available, falling back to the internal
ref:
```typescript
const internalGridRef = useRef<AgGridReact>(null);
const effectiveGridRef = gridRef || internalGridRef;
```
Then replace all `gridRef.current` references with
`effectiveGridRef.current` and pass `effectiveGridRef` to the ThemedAgGridReact
component.
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/627ec5ea-add3-47d6-a04d-bc19a8c8eaeb/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/627ec5ea-add3-47d6-a04d-bc19a8c8eaeb?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/627ec5ea-add3-47d6-a04d-bc19a8c8eaeb?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/627ec5ea-add3-47d6-a04d-bc19a8c8eaeb?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/627ec5ea-add3-47d6-a04d-bc19a8c8eaeb)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:3400a8ad-4a2b-4640-8e99-f8b488701240 -->
[](3400a8ad-4a2b-4640-8e99-f8b488701240)
##########
superset-frontend/src/dashboard/components/URLShortLinkButton/index.tsx:
##########
@@ -49,21 +49,39 @@ export default function URLShortLinkButton({
const theme = useTheme();
const [shortUrl, setShortUrl] = useState('');
const { addDangerToast } = useToasts();
- const { dataMask, activeTabs } = useSelector(
+ const { dataMask, activeTabs, chartStates, sliceEntities } = useSelector(
(state: RootState) => ({
dataMask: state.dataMask,
activeTabs: state.dashboardState.activeTabs,
+ chartStates: state.dashboardState.chartStates,
+ sliceEntities: state.sliceEntities,
}),
shallowEqual,
);
const getCopyUrl = async () => {
try {
+ // Check if dashboard has AG Grid tables (Table V2)
+ const hasAgGridTables =
+ sliceEntities &&
+ Object.values(sliceEntities).some(
+ slice =>
+ slice &&
+ typeof slice === 'object' &&
+ 'viz_type' in slice &&
+ slice.viz_type === 'ag_grid_table',
+ );
Review Comment:
### Violation of Single Responsibility Principle <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The logic for checking AG Grid tables is embedded within the URL generation
component, violating the Single Responsibility Principle.
###### Why this matters
This coupling makes the component harder to maintain and less reusable. The
component should focus on URL generation without needing to know the specific
implementation details of table types.
###### Suggested change ∙ *Feature Preview*
Extract the AG Grid table detection logic into a separate utility function
or service:
```typescript
// tableUtils.ts
export const hasAgGridTables = (sliceEntities: any) => {
return Object.values(sliceEntities).some(
slice => slice?.viz_type === 'ag_grid_table'
);
};
// URLShortLinkButton
import { hasAgGridTables } from './tableUtils';
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e21cfa76-9bd5-4732-8d88-f2a56b410744/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e21cfa76-9bd5-4732-8d88-f2a56b410744?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e21cfa76-9bd5-4732-8d88-f2a56b410744?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e21cfa76-9bd5-4732-8d88-f2a56b410744?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e21cfa76-9bd5-4732-8d88-f2a56b410744)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:f3e86ab9-d94e-4275-9a1c-16a31e93800b -->
[](f3e86ab9-d94e-4275-9a1c-16a31e93800b)
##########
superset-frontend/src/components/Chart/Chart.tsx:
##########
@@ -80,6 +80,7 @@ export interface ChartProps {
datasetsStatus?: 'loading' | 'error' | 'complete';
isInView?: boolean;
emitCrossFilters?: boolean;
+ onChartStateChange?: (chartState: JsonObject) => void;
Review Comment:
### Unwired chart state callback <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
New prop declared but not used to persist or propagate chart state; the
intended global storage and propagation of chart state is not implemented yet.
###### Why this matters
Declaring the callback without wiring into the chart's state changes
prevents persisting/restoring states via dashboard state/permalinks, meaning
the feature will have no effect at runtime and is likely to confuse future
maintainers.
###### Suggested change ∙ *Feature Preview*
Wire the callback into actual chart state changes. For example, when the
chart state changes inside the ChartRenderer, invoke the callback with the new
state. This can be achieved by:
```tsx
// In Chart.tsx or where the chart state is updated
// Ensure ChartRenderer accepts a callback prop
<ChartRenderer
{...this.props}
onChartStateChange={this.props.onChartStateChange}
// ...other props
/>
```
```ts
// In ChartRenderer.tsx (or equivalent)
interface ChartRendererProps {
// ... existing props
onChartStateChange?: (state: JsonObject) => void;
}
// When chart state updates
private updateChartState(state: JsonObject) {
this.setState({ chartState: state });
this.props.onChartStateChange?.(state);
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bcbfd801-5ad8-49ac-b23b-6fd837be9b19/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bcbfd801-5ad8-49ac-b23b-6fd837be9b19?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bcbfd801-5ad8-49ac-b23b-6fd837be9b19?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bcbfd801-5ad8-49ac-b23b-6fd837be9b19?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bcbfd801-5ad8-49ac-b23b-6fd837be9b19)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:6cb8ed06-d3bb-4bc8-922f-4c6c4d557a27 -->
[](6cb8ed06-d3bb-4bc8-922f-4c6c4d557a27)
--
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]
