korbit-ai[bot] commented on code in PR #35147:
URL: https://github.com/apache/superset/pull/35147#discussion_r2350195663
##########
superset-frontend/src/dashboard/util/buildFilterScopeTreeEntry.ts:
##########
@@ -16,17 +16,41 @@
* specific language governing permissions and limitations
* under the License.
*/
+import { DashboardLayout } from '../types';
import getFilterScopeNodesTree from './getFilterScopeNodesTree';
import getFilterScopeParentNodes from './getFilterScopeParentNodes';
import getKeyForFilterScopeTree from './getKeyForFilterScopeTree';
import getSelectedChartIdForFilterScopeTree from
'./getSelectedChartIdForFilterScopeTree';
+interface FilterScopeMapItem {
+ checked?: number[];
+ expanded?: string[];
+}
Review Comment:
### Type inconsistency between related data structures <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Inconsistent type usage between FilterScopeMapItem.checked (number[]) and
the checkedChartIdSet which stores string values.
###### Why this matters
Type inconsistency can lead to runtime errors and requires implicit type
conversions, making the code more error-prone and harder to maintain.
###### Suggested change ∙ *Feature Preview*
Align types across related structures:
```typescript
interface FilterScopeMapItem {
checked?: string[];
expanded?: string[];
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b92dda7b-7747-4a7a-90ea-6cc479e23e35/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b92dda7b-7747-4a7a-90ea-6cc479e23e35?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b92dda7b-7747-4a7a-90ea-6cc479e23e35?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b92dda7b-7747-4a7a-90ea-6cc479e23e35?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b92dda7b-7747-4a7a-90ea-6cc479e23e35)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:6328cde0-0620-4901-969d-ea1267203417 -->
[](6328cde0-0620-4901-969d-ea1267203417)
##########
superset-frontend/src/dashboard/reducers/sliceEntities.ts:
##########
@@ -23,60 +23,55 @@
FETCH_ALL_SLICES_STARTED,
ADD_SLICES,
SET_SLICES,
+ SliceEntitiesState,
+ SliceEntitiesActionPayload,
} from '../actions/sliceEntities';
import { HYDRATE_DASHBOARD } from '../actions/hydrate';
-export const initSliceEntities = {
+export const initSliceEntities: SliceEntitiesState = {
slices: {},
isLoading: true,
errorMessage: null,
lastUpdated: 0,
};
export default function sliceEntitiesReducer(
- state = initSliceEntities,
- action,
-) {
- const actionHandlers = {
- [HYDRATE_DASHBOARD]() {
+ state: SliceEntitiesState = initSliceEntities,
+ action: SliceEntitiesActionPayload,
+): SliceEntitiesState {
+ switch (action.type) {
+ case HYDRATE_DASHBOARD:
return {
...action.data.sliceEntities,
};
- },
- [FETCH_ALL_SLICES_STARTED]() {
+ case FETCH_ALL_SLICES_STARTED:
return {
...state,
isLoading: true,
};
- },
- [ADD_SLICES]() {
+ case ADD_SLICES:
return {
...state,
isLoading: false,
slices: { ...state.slices, ...action.payload.slices },
lastUpdated: new Date().getTime(),
Review Comment:
### Redundant Date object creation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Multiple action handlers call new Date().getTime() which creates unnecessary
Date objects and performs redundant timestamp calculations.
###### Why this matters
Creating new Date objects repeatedly adds overhead and can cause slight
timing inconsistencies between actions processed in the same batch. This
impacts performance in high-frequency scenarios.
###### Suggested change ∙ *Feature Preview*
Calculate the timestamp once at the beginning of the reducer function and
reuse it:
```typescript
export default function sliceEntitiesReducer(
state: SliceEntitiesState = initSliceEntities,
action: SliceEntitiesActionPayload,
): SliceEntitiesState {
const timestamp = new Date().getTime();
switch (action.type) {
// ... other cases
case ADD_SLICES:
return {
...state,
isLoading: false,
slices: { ...state.slices, ...action.payload.slices },
lastUpdated: timestamp,
};
case SET_SLICES:
return {
...state,
isLoading: false,
slices: { ...action.payload.slices },
lastUpdated: timestamp,
};
case FETCH_ALL_SLICES_FAILED:
return {
...state,
isLoading: false,
lastUpdated: timestamp,
errorMessage:
action.payload.error || t('Could not fetch all saved charts'),
};
default:
return state;
}
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d8147a8d-5f8d-479e-a6c4-cea0407269ef/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d8147a8d-5f8d-479e-a6c4-cea0407269ef?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d8147a8d-5f8d-479e-a6c4-cea0407269ef?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d8147a8d-5f8d-479e-a6c4-cea0407269ef?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d8147a8d-5f8d-479e-a6c4-cea0407269ef)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:af81a2e0-abb7-4000-a4cd-2598f5d98d64 -->
[](af81a2e0-abb7-4000-a4cd-2598f5d98d64)
##########
superset-frontend/src/dashboard/util/findFirstParentContainer.ts:
##########
@@ -18,8 +18,11 @@
*/
import { TABS_TYPE } from './componentTypes';
import { DASHBOARD_ROOT_ID } from './constants';
+import { DashboardLayout } from '../types';
-export default function findFirstParentContainerId(layout = {}) {
+export default function findFirstParentContainerId(
+ layout: DashboardLayout = {},
+): string {
// DASHBOARD_GRID_TYPE or TABS_TYPE?
let parent = layout[DASHBOARD_ROOT_ID];
Review Comment:
### Unclear Function Purpose and Implementation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The function name and implementation don't clearly communicate the business
logic or purpose of finding the first parent container.
###### Why this matters
Unclear naming and lack of documentation makes the code harder to maintain
and understand, especially since it handles special cases for tab layouts.
###### Suggested change ∙ *Feature Preview*
Rename the function to better reflect its purpose (e.g.,
`findFirstContentContainer`) and add JSDoc documentation explaining the logic:
```typescript
/**
* Finds the first content container in the dashboard layout.
* If the root's first child is a tabs container, returns the first tab's
container.
* Otherwise, returns the first child container of the root.
* @param layout - The dashboard layout structure
* @returns The ID of the first content container
*/
export default function findFirstContentContainer(
layout: DashboardLayout = {},
): string {
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c109ee7d-7787-4ea9-a9ad-986699af2c54/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c109ee7d-7787-4ea9-a9ad-986699af2c54?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c109ee7d-7787-4ea9-a9ad-986699af2c54?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c109ee7d-7787-4ea9-a9ad-986699af2c54?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c109ee7d-7787-4ea9-a9ad-986699af2c54)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:2bb39ef8-b884-45dd-a5f9-ad89d095b3dc -->
[](2bb39ef8-b884-45dd-a5f9-ad89d095b3dc)
##########
superset-frontend/src/dashboard/reducers/sliceEntities.ts:
##########
@@ -23,60 +23,55 @@ import {
FETCH_ALL_SLICES_STARTED,
ADD_SLICES,
SET_SLICES,
+ SliceEntitiesState,
+ SliceEntitiesActionPayload,
} from '../actions/sliceEntities';
import { HYDRATE_DASHBOARD } from '../actions/hydrate';
-export const initSliceEntities = {
+export const initSliceEntities: SliceEntitiesState = {
slices: {},
isLoading: true,
errorMessage: null,
lastUpdated: 0,
};
export default function sliceEntitiesReducer(
- state = initSliceEntities,
- action,
-) {
- const actionHandlers = {
- [HYDRATE_DASHBOARD]() {
+ state: SliceEntitiesState = initSliceEntities,
+ action: SliceEntitiesActionPayload,
+): SliceEntitiesState {
+ switch (action.type) {
+ case HYDRATE_DASHBOARD:
return {
...action.data.sliceEntities,
};
- },
- [FETCH_ALL_SLICES_STARTED]() {
+ case FETCH_ALL_SLICES_STARTED:
return {
...state,
isLoading: true,
};
- },
- [ADD_SLICES]() {
+ case ADD_SLICES:
return {
...state,
isLoading: false,
slices: { ...state.slices, ...action.payload.slices },
lastUpdated: new Date().getTime(),
};
- },
- [SET_SLICES]() {
+ case SET_SLICES:
return {
+ ...state,
isLoading: false,
slices: { ...action.payload.slices },
lastUpdated: new Date().getTime(),
};
Review Comment:
### Similar State Update Patterns <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The SET_SLICES and ADD_SLICES cases share similar state updates with only
slight differences in how slices are handled.
###### Why this matters
Similar state update patterns across different cases indicate potential for
shared logic extraction, improving maintainability and reducing code
duplication.
###### Suggested change ∙ *Feature Preview*
Extract common state updates into a helper function:
```typescript
const updateSlicesState = (state: SliceEntitiesState, slices: object) => ({
...state,
isLoading: false,
slices,
lastUpdated: getCurrentTimestamp(),
});
// Then in reducer:
case SET_SLICES:
return updateSlicesState(state, action.payload.slices);
case ADD_SLICES:
return updateSlicesState(state, { ...state.slices,
...action.payload.slices });
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4cc22f56-0d1e-43b2-b795-e93df3f599bc/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4cc22f56-0d1e-43b2-b795-e93df3f599bc?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4cc22f56-0d1e-43b2-b795-e93df3f599bc?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4cc22f56-0d1e-43b2-b795-e93df3f599bc?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4cc22f56-0d1e-43b2-b795-e93df3f599bc)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:82e50d7e-2fd1-4a13-85ee-b097bef8071d -->
[](82e50d7e-2fd1-4a13-85ee-b097bef8071d)
##########
superset-frontend/src/dashboard/util/getEmptyLayout.ts:
##########
@@ -17,14 +17,30 @@
* under the License.
*/
import { DASHBOARD_ROOT_TYPE, DASHBOARD_GRID_TYPE } from './componentTypes';
+import type { ComponentType } from '../types';
import {
DASHBOARD_GRID_ID,
DASHBOARD_ROOT_ID,
DASHBOARD_VERSION_KEY,
} from './constants';
-export default function getEmptyLayout() {
+// Basic layout item for empty dashboard (simplified version without meta)
+interface BasicLayoutItem {
+ type: ComponentType;
+ id: string;
+ children: string[];
+ parents?: string[];
+}
+
+// Empty layout structure
+type EmptyLayout = {
+ [DASHBOARD_VERSION_KEY]: string;
+ [DASHBOARD_ROOT_ID]: BasicLayoutItem;
+ [DASHBOARD_GRID_ID]: BasicLayoutItem;
+};
Review Comment:
### Unsafe computed property type definition <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The EmptyLayout type uses computed property names with constants, but
TypeScript cannot guarantee these constants won't conflict with other string
keys at compile time.
###### Why this matters
This creates a type that is not truly type-safe because other string keys
could potentially override the expected structure, and the type system cannot
prevent runtime key collisions.
###### Suggested change ∙ *Feature Preview*
Use a more explicit type definition with string literal types or template
literal types:
```typescript
type EmptyLayout = {
[K in typeof DASHBOARD_VERSION_KEY | typeof DASHBOARD_ROOT_ID | typeof
DASHBOARD_GRID_ID]:
K extends typeof DASHBOARD_VERSION_KEY ? string :
K extends typeof DASHBOARD_ROOT_ID ? BasicLayoutItem :
K extends typeof DASHBOARD_GRID_ID ? BasicLayoutItem :
never;
};
```
Or use a simpler record type:
```typescript
type EmptyLayout = Record<typeof DASHBOARD_VERSION_KEY, string> &
Record<typeof DASHBOARD_ROOT_ID, BasicLayoutItem> &
Record<typeof DASHBOARD_GRID_ID, BasicLayoutItem>;
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/60fe1cb6-f5d4-4648-9afe-0785264b0ba2/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/60fe1cb6-f5d4-4648-9afe-0785264b0ba2?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/60fe1cb6-f5d4-4648-9afe-0785264b0ba2?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/60fe1cb6-f5d4-4648-9afe-0785264b0ba2?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/60fe1cb6-f5d4-4648-9afe-0785264b0ba2)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:c11e4313-8022-46c8-9316-17ab8045558d -->
[](c11e4313-8022-46c8-9316-17ab8045558d)
##########
superset-frontend/src/dashboard/util/buildFilterScopeTreeEntry.ts:
##########
@@ -16,17 +16,41 @@
* specific language governing permissions and limitations
* under the License.
*/
+import { DashboardLayout } from '../types';
import getFilterScopeNodesTree from './getFilterScopeNodesTree';
import getFilterScopeParentNodes from './getFilterScopeParentNodes';
import getKeyForFilterScopeTree from './getKeyForFilterScopeTree';
import getSelectedChartIdForFilterScopeTree from
'./getSelectedChartIdForFilterScopeTree';
+interface FilterScopeMapItem {
+ checked?: number[];
+ expanded?: string[];
+}
+
+interface FilterScopeMap {
+ [key: string]: FilterScopeMapItem;
+}
+
+interface FilterScopeTreeEntry {
+ nodes: any[];
+ nodesFiltered: any[];
+ checked: string[];
+ expanded: string[];
+}
Review Comment:
### Avoid any[] type in interface definition <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The interface uses 'any[]' type for both nodes and nodesFiltered properties,
which violates type safety principles and makes the code less maintainable.
###### Why this matters
Using 'any' type bypasses TypeScript's type checking system, making it
harder to catch errors during development and complicating future refactoring
efforts.
###### Suggested change ∙ *Feature Preview*
Define a proper type for the nodes structure:
```typescript
interface FilterScopeNode {
value: string;
title: string;
children?: FilterScopeNode[];
// add other necessary properties
}
interface FilterScopeTreeEntry {
nodes: FilterScopeNode[];
nodesFiltered: FilterScopeNode[];
checked: string[];
expanded: string[];
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7c179bb7-b9ef-4ae9-97b6-fbfdd6419d16/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7c179bb7-b9ef-4ae9-97b6-fbfdd6419d16?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7c179bb7-b9ef-4ae9-97b6-fbfdd6419d16?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7c179bb7-b9ef-4ae9-97b6-fbfdd6419d16?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7c179bb7-b9ef-4ae9-97b6-fbfdd6419d16)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:15d06035-4767-435f-ab1a-3c813b799bc6 -->
[](15d06035-4767-435f-ab1a-3c813b799bc6)
##########
superset-frontend/src/dashboard/util/getLeafComponentIdFromPath.ts:
##########
@@ -18,15 +18,20 @@
*/
import { IN_COMPONENT_ELEMENT_TYPES } from './constants';
-export default function getLeafComponentIdFromPath(directPathToChild = []) {
+export default function getLeafComponentIdFromPath(
+ directPathToChild: string[] = [],
+): string | null {
if (directPathToChild.length > 0) {
const currentPath = directPathToChild.slice();
while (currentPath.length) {
const componentId = currentPath.pop();
const componentType = componentId && componentId.split('-')[0];
Review Comment:
### Function violates Single Responsibility Principle <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The function mixes multiple responsibilities: path traversal, component ID
extraction, and component type validation. This violates the Single
Responsibility Principle.
###### Why this matters
Complex functions with multiple responsibilities are harder to test,
maintain, and reuse. Changes to one aspect might affect others unexpectedly.
###### Suggested change ∙ *Feature Preview*
Split the function into smaller, focused functions:
```typescript
function extractComponentType(componentId: string): string {
return componentId.split('-')[0];
}
function isLeafComponent(componentType: string): boolean {
return !IN_COMPONENT_ELEMENT_TYPES.includes(componentType);
}
export default function getLeafComponentIdFromPath(directPathToChild:
string[] = []): string | null {
for (let i = directPathToChild.length - 1; i >= 0; i--) {
const componentId = directPathToChild[i];
const componentType = extractComponentType(componentId);
if (isLeafComponent(componentType)) {
return componentId;
}
}
return null;
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6ac54a02-b092-4f1f-a173-c8819fa38bcb/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6ac54a02-b092-4f1f-a173-c8819fa38bcb?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6ac54a02-b092-4f1f-a173-c8819fa38bcb?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6ac54a02-b092-4f1f-a173-c8819fa38bcb?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6ac54a02-b092-4f1f-a173-c8819fa38bcb)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:d7af5885-fc29-4964-be5b-fd0922af0cd0 -->
[](d7af5885-fc29-4964-be5b-fd0922af0cd0)
##########
superset-frontend/src/dashboard/reducers/sliceEntities.ts:
##########
@@ -23,60 +23,55 @@
FETCH_ALL_SLICES_STARTED,
ADD_SLICES,
SET_SLICES,
+ SliceEntitiesState,
+ SliceEntitiesActionPayload,
} from '../actions/sliceEntities';
import { HYDRATE_DASHBOARD } from '../actions/hydrate';
-export const initSliceEntities = {
+export const initSliceEntities: SliceEntitiesState = {
slices: {},
isLoading: true,
errorMessage: null,
lastUpdated: 0,
};
export default function sliceEntitiesReducer(
- state = initSliceEntities,
- action,
-) {
- const actionHandlers = {
- [HYDRATE_DASHBOARD]() {
+ state: SliceEntitiesState = initSliceEntities,
+ action: SliceEntitiesActionPayload,
+): SliceEntitiesState {
+ switch (action.type) {
+ case HYDRATE_DASHBOARD:
return {
...action.data.sliceEntities,
};
Review Comment:
### Missing state preservation in HYDRATE_DASHBOARD case <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The HYDRATE_DASHBOARD case does not preserve the state structure and may
lose required properties from SliceEntitiesState.
###### Why this matters
This could cause runtime errors or unexpected behavior if
action.data.sliceEntities is missing required properties like isLoading,
errorMessage, or lastUpdated that are defined in SliceEntitiesState.
###### Suggested change ∙ *Feature Preview*
Preserve the existing state structure by spreading the current state first:
```typescript
case HYDRATE_DASHBOARD:
return {
...state,
...action.data.sliceEntities,
};
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cffbf411-2a67-4389-a173-409c0601b1b1/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cffbf411-2a67-4389-a173-409c0601b1b1?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cffbf411-2a67-4389-a173-409c0601b1b1?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cffbf411-2a67-4389-a173-409c0601b1b1?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cffbf411-2a67-4389-a173-409c0601b1b1)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:5459e12e-e76a-4e7c-b8c8-5a62a8ccde49 -->
[](5459e12e-e76a-4e7c-b8c8-5a62a8ccde49)
##########
superset-frontend/src/dashboard/reducers/sliceEntities.ts:
##########
@@ -23,60 +23,55 @@
FETCH_ALL_SLICES_STARTED,
ADD_SLICES,
SET_SLICES,
+ SliceEntitiesState,
+ SliceEntitiesActionPayload,
} from '../actions/sliceEntities';
import { HYDRATE_DASHBOARD } from '../actions/hydrate';
-export const initSliceEntities = {
+export const initSliceEntities: SliceEntitiesState = {
slices: {},
isLoading: true,
errorMessage: null,
lastUpdated: 0,
};
export default function sliceEntitiesReducer(
- state = initSliceEntities,
- action,
-) {
- const actionHandlers = {
- [HYDRATE_DASHBOARD]() {
+ state: SliceEntitiesState = initSliceEntities,
+ action: SliceEntitiesActionPayload,
+): SliceEntitiesState {
+ switch (action.type) {
+ case HYDRATE_DASHBOARD:
return {
...action.data.sliceEntities,
};
- },
- [FETCH_ALL_SLICES_STARTED]() {
+ case FETCH_ALL_SLICES_STARTED:
return {
...state,
isLoading: true,
};
- },
- [ADD_SLICES]() {
+ case ADD_SLICES:
return {
...state,
isLoading: false,
slices: { ...state.slices, ...action.payload.slices },
lastUpdated: new Date().getTime(),
Review Comment:
### Duplicated Timestamp Logic <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The timestamp generation is duplicated across multiple reducer cases
(ADD_SLICES, SET_SLICES, FETCH_ALL_SLICES_FAILED).
###### Why this matters
Code duplication increases maintenance burden and risk of inconsistencies if
the timestamp generation logic needs to change.
###### Suggested change ∙ *Feature Preview*
Extract the timestamp generation into a utility function or constant:
```typescript
const getCurrentTimestamp = () => new Date().getTime();
// Then in reducer cases:
lastUpdated: getCurrentTimestamp(),
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/da4cbccd-7c33-47e0-ab48-964a7c5de71b/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/da4cbccd-7c33-47e0-ab48-964a7c5de71b?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/da4cbccd-7c33-47e0-ab48-964a7c5de71b?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/da4cbccd-7c33-47e0-ab48-964a7c5de71b?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/da4cbccd-7c33-47e0-ab48-964a7c5de71b)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:3bb0c59e-c2da-4f32-9531-2be653946743 -->
[](3bb0c59e-c2da-4f32-9531-2be653946743)
##########
superset-frontend/src/dashboard/util/buildFilterScopeTreeEntry.ts:
##########
@@ -43,15 +67,15 @@
filterFields: editingList,
selectedChartId,
});
- const checkedChartIdSet = new Set();
+ const checkedChartIdSet = new Set<string>();
editingList.forEach(filterField => {
- (filterScopeMap[filterField].checked || []).forEach(chartId => {
+ (filterScopeMap[filterField]?.checked || []).forEach(chartId => {
checkedChartIdSet.add(`${chartId}:${filterField}`);
});
});
Review Comment:
### Inefficient nested forEach loops <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Nested forEach loops create O(n*m) time complexity when a single loop with
flatMap could achieve the same result more efficiently.
###### Why this matters
This nested iteration pattern scales poorly as the number of filter fields
and checked items grows, potentially causing performance degradation in
dashboards with many filters.
###### Suggested change ∙ *Feature Preview*
Replace nested forEach with a single flatMap operation:
```typescript
const checkedChartIdSet = new Set(
editingList.flatMap(filterField =>
(filterScopeMap[filterField]?.checked || []).map(chartId =>
`${chartId}:${filterField}`
)
)
);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/406ba7bb-e7a1-49a4-94df-696d9b056d16/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/406ba7bb-e7a1-49a4-94df-696d9b056d16?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/406ba7bb-e7a1-49a4-94df-696d9b056d16?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/406ba7bb-e7a1-49a4-94df-696d9b056d16?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/406ba7bb-e7a1-49a4-94df-696d9b056d16)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:b765eba3-295a-4602-94c0-43ebb50ee19c -->
[](b765eba3-295a-4602-94c0-43ebb50ee19c)
--
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]
