korbit-ai[bot] commented on code in PR #33106:
URL: https://github.com/apache/superset/pull/33106#discussion_r2040432398
##########
superset-frontend/src/dashboard/reducers/dashboardState.js:
##########
@@ -209,12 +209,17 @@ export default function dashboardStateReducer(state = {},
action) {
};
},
[SET_ACTIVE_TAB]() {
- const newActiveTabs = new Set(state.activeTabs);
- newActiveTabs.delete(action.prevTabId);
- newActiveTabs.add(action.tabId);
+ const newActiveTabs = new Set(state.activeTabs).difference(
+ new Set(action.inactiveTabs.concat(action.prevTabId)),
+ );
Review Comment:
### Invalid Set operation method <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Using the Set.difference() method which does not exist in JavaScript's
native Set implementation. This will cause a runtime error.
###### Why this matters
The code will throw a TypeError when executed as 'difference' is not a
method of the JavaScript Set object, causing the tab management functionality
to break.
###### Suggested change ∙ *Feature Preview*
Replace with standard Set operations using Array.filter(). Here's the
corrected version:
```javascript
const newActiveTabs = new Set(
Array.from(state.activeTabs).filter(
tab => !action.inactiveTabs.includes(tab) && tab !== action.prevTabId
)
);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/12e58d69-c1ce-4211-b1d6-fc0722d6bc25/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/12e58d69-c1ce-4211-b1d6-fc0722d6bc25?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/12e58d69-c1ce-4211-b1d6-fc0722d6bc25?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/12e58d69-c1ce-4211-b1d6-fc0722d6bc25?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/12e58d69-c1ce-4211-b1d6-fc0722d6bc25)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:e0f3d330-7f3e-4977-baf7-61d5dfe2fc84 -->
[](e0f3d330-7f3e-4977-baf7-61d5dfe2fc84)
##########
superset-frontend/src/dashboard/actions/dashboardState.js:
##########
@@ -659,7 +659,41 @@ export function setDirectPathToChild(path) {
export const SET_ACTIVE_TAB = 'SET_ACTIVE_TAB';
export function setActiveTab(tabId, prevTabId) {
- return { type: SET_ACTIVE_TAB, tabId, prevTabId };
+ return (dispatch, getState) => {
+ const { dashboardLayout, dashboardState } = getState();
+ const { activeTabs, inactiveTabs: prevInactiveTabs } = dashboardState;
+ const { present: currentLayout } = dashboardLayout;
+ const restoredTabs = [];
+ const queue = [tabId];
+ while (queue.length > 0) {
+ const seek = queue.shift();
+ const found =
+ prevInactiveTabs?.filter(inactiveTabId =>
+ currentLayout[inactiveTabId]?.parents
+ .filter(id => id.startsWith('TAB-'))
+ .slice(-1)
+ .includes(seek),
+ ) ?? [];
+
+ restoredTabs.push(...found);
+ queue.push(...found);
+ }
Review Comment:
### Potential infinite loop in tab restoration <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The tab restoration logic may enter an infinite loop if there is a circular
parent-child relationship in the tab structure.
###### Why this matters
This could cause the browser to hang or crash if there are any cycles in the
tab hierarchy, severely impacting user experience.
###### Suggested change ∙ *Feature Preview*
Add a visited set to prevent processing the same tab multiple times:
```javascript
const restoredTabs = [];
const queue = [tabId];
const visited = new Set([tabId]);
while (queue.length > 0) {
const seek = queue.shift();
const found =
prevInactiveTabs?.filter(inactiveTabId =>
currentLayout[inactiveTabId]?.parents
.filter(id => id.startsWith('TAB-'))
.slice(-1)
.includes(seek),
) ?? [];
for (const tab of found) {
if (!visited.has(tab)) {
visited.add(tab);
restoredTabs.push(tab);
queue.push(tab);
}
}
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3a441ade-8961-4120-adbe-ffdfe04c0bd2/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3a441ade-8961-4120-adbe-ffdfe04c0bd2?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3a441ade-8961-4120-adbe-ffdfe04c0bd2?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3a441ade-8961-4120-adbe-ffdfe04c0bd2?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3a441ade-8961-4120-adbe-ffdfe04c0bd2)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:7355396a-e336-425b-af30-4d13321aeaf7 -->
[](7355396a-e336-425b-af30-4d13321aeaf7)
##########
superset-frontend/src/dashboard/actions/dashboardState.js:
##########
@@ -659,7 +659,41 @@
export const SET_ACTIVE_TAB = 'SET_ACTIVE_TAB';
export function setActiveTab(tabId, prevTabId) {
- return { type: SET_ACTIVE_TAB, tabId, prevTabId };
+ return (dispatch, getState) => {
+ const { dashboardLayout, dashboardState } = getState();
+ const { activeTabs, inactiveTabs: prevInactiveTabs } = dashboardState;
+ const { present: currentLayout } = dashboardLayout;
+ const restoredTabs = [];
+ const queue = [tabId];
+ while (queue.length > 0) {
+ const seek = queue.shift();
+ const found =
+ prevInactiveTabs?.filter(inactiveTabId =>
+ currentLayout[inactiveTabId]?.parents
+ .filter(id => id.startsWith('TAB-'))
+ .slice(-1)
+ .includes(seek),
+ ) ?? [];
+
+ restoredTabs.push(...found);
+ queue.push(...found);
+ }
+ const tabIds = restoredTabs ? [tabId].concat(restoredTabs) : [tabId];
+ const inactiveTabs =
+ Boolean(prevTabId) && tabId !== prevTabId
+ ? activeTabs.filter(
+ activeTabId =>
+ activeTabId !== prevTabId &&
+ currentLayout[activeTabId]?.parents.includes(prevTabId),
+ )
+ : [];
Review Comment:
### Unclear Tab Inactivation Logic <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The logic for determining inactive tabs uses multiple conditions in a
ternary operator combined with a filter, making it hard to understand the
criteria for tab inactivation.
###### Why this matters
Complex boolean logic in a single expression makes it difficult to
understand and modify the tab inactivation rules.
###### Suggested change ∙ *Feature Preview*
```javascript
const shouldProcessInactiveTabs = Boolean(prevTabId) && tabId !== prevTabId;
const getInactiveTabsForPrevTab = () =>
activeTabs.filter(activeTabId =>
activeTabId !== prevTabId &&
currentLayout[activeTabId]?.parents.includes(prevTabId)
);
const inactiveTabs = shouldProcessInactiveTabs ? getInactiveTabsForPrevTab()
: [];
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/04c91584-b153-41d3-be14-f5f8e4ba3bf5/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/04c91584-b153-41d3-be14-f5f8e4ba3bf5?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/04c91584-b153-41d3-be14-f5f8e4ba3bf5?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/04c91584-b153-41d3-be14-f5f8e4ba3bf5?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/04c91584-b153-41d3-be14-f5f8e4ba3bf5)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:df971131-8c71-4166-8a64-cda093bacd8b -->
[](df971131-8c71-4166-8a64-cda093bacd8b)
##########
superset-frontend/src/dashboard/actions/dashboardState.js:
##########
@@ -659,7 +659,41 @@
export const SET_ACTIVE_TAB = 'SET_ACTIVE_TAB';
export function setActiveTab(tabId, prevTabId) {
- return { type: SET_ACTIVE_TAB, tabId, prevTabId };
+ return (dispatch, getState) => {
+ const { dashboardLayout, dashboardState } = getState();
+ const { activeTabs, inactiveTabs: prevInactiveTabs } = dashboardState;
+ const { present: currentLayout } = dashboardLayout;
+ const restoredTabs = [];
+ const queue = [tabId];
+ while (queue.length > 0) {
+ const seek = queue.shift();
+ const found =
+ prevInactiveTabs?.filter(inactiveTabId =>
+ currentLayout[inactiveTabId]?.parents
+ .filter(id => id.startsWith('TAB-'))
+ .slice(-1)
+ .includes(seek),
+ ) ?? [];
+
+ restoredTabs.push(...found);
+ queue.push(...found);
+ }
Review Comment:
### Complex Business Logic in Action Creator <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The tab restoration logic is embedded directly in the action creator,
violating Single Responsibility Principle and making the code harder to test
and maintain.
###### Why this matters
Having complex business logic in action creators makes the code harder to
test, maintain, and reuse. The tab restoration logic should be extracted into a
separate service or utility function.
###### Suggested change ∙ *Feature Preview*
Extract the tab restoration logic into a separate utility function:
```javascript
const findTabsToRestore = (tabId, prevInactiveTabs, currentLayout) => {
const restoredTabs = [];
const queue = [tabId];
while (queue.length > 0) {
const seek = queue.shift();
const found = prevInactiveTabs?.filter(/* ... */) ?? [];
restoredTabs.push(...found);
queue.push(...found);
}
return restoredTabs;
};
export function setActiveTab(tabId, prevTabId) {
return (dispatch, getState) => {
const { dashboardLayout, dashboardState } = getState();
const restoredTabs = findTabsToRestore(tabId,
dashboardState.inactiveTabs, dashboardLayout.present);
// ... rest of the logic
};
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b481cd3e-8b16-486d-b4e9-3ca8ccd65092/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b481cd3e-8b16-486d-b4e9-3ca8ccd65092?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b481cd3e-8b16-486d-b4e9-3ca8ccd65092?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b481cd3e-8b16-486d-b4e9-3ca8ccd65092?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b481cd3e-8b16-486d-b4e9-3ca8ccd65092)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:adf02420-94db-4faf-a7ad-d04744c07a92 -->
[](adf02420-94db-4faf-a7ad-d04744c07a92)
##########
superset-frontend/src/dashboard/reducers/dashboardState.js:
##########
@@ -209,12 +209,17 @@
};
},
[SET_ACTIVE_TAB]() {
- const newActiveTabs = new Set(state.activeTabs);
- newActiveTabs.delete(action.prevTabId);
- newActiveTabs.add(action.tabId);
+ const newActiveTabs = new Set(state.activeTabs).difference(
+ new Set(action.inactiveTabs.concat(action.prevTabId)),
+ );
+ const newInactiveTabs = new Set(state.inactiveTabs)
+ .difference(new Set(action.tabIds))
+ .union(new Set(action.inactiveTabs));
Review Comment:
### Complex Set Operations Chain <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The Set operations chain is unnecessarily complex and hard to follow, making
the tab state management logic difficult to understand at a glance.
###### Why this matters
Complex chaining of Set operations reduces code maintainability and makes
debugging tab state issues more challenging.
###### Suggested change ∙ *Feature Preview*
```javascript
// First, create the sets
const inactiveTabsSet = new Set(action.inactiveTabs);
const prevTabSet = new Set([action.prevTabId]);
const tabIdsSet = new Set(action.tabIds);
// Then perform operations clearly
const newActiveTabs = new Set(state.activeTabs);
// Remove all inactive tabs and previous tab
inactiveTabsSet.forEach(tab => newActiveTabs.delete(tab));
newActiveTabs.delete(action.prevTabId);
// Add new tab IDs
tabIdsSet.forEach(tab => newActiveTabs.add(tab));
// Similarly for inactive tabs
const newInactiveTabs = new Set(state.inactiveTabs);
tabIdsSet.forEach(tab => newInactiveTabs.delete(tab));
inactiveTabsSet.forEach(tab => newInactiveTabs.add(tab));
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/94ffd1a7-0fed-42e6-9475-f72a6f426114/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/94ffd1a7-0fed-42e6-9475-f72a6f426114?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/94ffd1a7-0fed-42e6-9475-f72a6f426114?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/94ffd1a7-0fed-42e6-9475-f72a6f426114?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/94ffd1a7-0fed-42e6-9475-f72a6f426114)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:2afb136e-8e7d-4b96-abfc-fd51653084cf -->
[](2afb136e-8e7d-4b96-abfc-fd51653084cf)
--
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]
