korbit-ai[bot] commented on code in PR #35265: URL: https://github.com/apache/superset/pull/35265#discussion_r2376027900
########## superset-frontend/src/dashboard/util/getChartIdsFromComponent.ts: ########## @@ -0,0 +1,44 @@ +/** + * 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. + */ +import { CHART_TYPE } from './componentTypes'; +import type { DashboardLayout } from '../types'; + +export default function getChartIdsFromComponent( + componentId: string, + layout: DashboardLayout, +): number[] { Review Comment: ### High coupling with DashboardLayout implementation <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The function is tightly coupled to the DashboardLayout type and specific component structure, making it less reusable and harder to test. ###### Why this matters This tight coupling makes the code less flexible for future changes and makes unit testing more complex as it requires setting up complete DashboardLayout objects. ###### Suggested change ∙ *Feature Preview* Consider creating an interface for the minimum required component structure and use that instead: ```typescript interface ComponentWithChildren { type?: string; meta?: { chartId?: number }; children?: string[]; } export default function getChartIdsFromComponent( componentId: string, layout: Record<string, ComponentWithChildren>, ): number[] { ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fbb537ab-ef2f-423f-be97-8e2bb61b22a7/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fbb537ab-ef2f-423f-be97-8e2bb61b22a7?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fbb537ab-ef2f-423f-be97-8e2bb61b22a7?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fbb537ab-ef2f-423f-be97-8e2bb61b22a7?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fbb537ab-ef2f-423f-be97-8e2bb61b22a7) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:e0de7d25-4286-4392-a321-67e1ffe2a10d --> [](e0de7d25-4286-4392-a321-67e1ffe2a10d) ########## superset-frontend/src/dashboard/util/charts/useAllChartIds.ts: ########## @@ -0,0 +1,29 @@ +/** + * 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. + */ +import { useSelector } from 'react-redux'; +import { useMemo } from 'react'; +import { RootState } from 'src/dashboard/types'; +import getChartIdsFromLayout from '../getChartIdsFromLayout'; + +export const useAllChartIds = () => { + const layout = useSelector( + (state: RootState) => state.dashboardLayout.present, + ); + return useMemo(() => getChartIdsFromLayout(layout), [layout]); Review Comment: ### Inefficient memoization key for layout <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? useMemo uses a large layout object as its dependency, which can cause recomputation on most layout changes. ###### Why this matters In Redux-based dashboards, the layout object is often replaced with a new reference on updates. Using the entire layout as the memoization key means getChartIdsFromLayout will run frequently, even if the relevant data for chart IDs did not change, leading to unnecessary work and potential UI latency. ###### Suggested change ∙ *Feature Preview* Refactor to memoize based on the specific parts of the layout that affect chart IDs, or switch to a memoized selector that computes IDs only when those sub-parts change. Example: ```ts import { createSelector } from 'reselect'; const selectLayout = (state: RootState) => state.dashboardLayout.present; const selectChartIds = createSelector([selectLayout], (layout) => getChartIdsFromLayout(layout)); export const useAllChartIds = () => { return useSelector(selectChartIds); }; ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0e963d86-3291-46c7-9116-57c0e9f4623c/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0e963d86-3291-46c7-9116-57c0e9f4623c?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0e963d86-3291-46c7-9116-57c0e9f4623c?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0e963d86-3291-46c7-9116-57c0e9f4623c?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0e963d86-3291-46c7-9116-57c0e9f4623c) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:458692ee-3395-45c0-9327-8d73a03d8135 --> [](458692ee-3395-45c0-9327-8d73a03d8135) ########## superset-frontend/src/dashboard/components/gridComponents/Tab/Tab.jsx: ########## @@ -113,6 +114,49 @@ const renderDraggableContent = dropProps => const Tab = props => { const dispatch = useDispatch(); const canEdit = useSelector(state => state.dashboardInfo.dash_edit_perm); + const dashboardLayout = useSelector(state => state.dashboardLayout.present); + const lastRefreshTime = useSelector( + state => state.dashboardState.lastRefreshTime, + ); + const tabActivationTimes = useSelector( + state => state.dashboardState.tabActivationTimes || {}, + ); + const dashboardInfo = useSelector(state => state.dashboardInfo); + + // Check if tab needs refresh when it becomes visible + useEffect(() => { + if (props.renderType === RENDER_TAB_CONTENT && props.isComponentVisible) { + const tabId = props.id; + const tabActivationTime = tabActivationTimes[tabId] || 0; + + // If a refresh occurred while this tab was inactive, + // refresh the charts in this tab now that it's visible + if ( + lastRefreshTime && + tabActivationTime && + lastRefreshTime > tabActivationTime + ) { + const chartIds = getChartIdsFromComponent(tabId, dashboardLayout); + if (chartIds.length > 0) { + // Small delay to ensure charts are fully mounted + setTimeout(() => { + // Refresh charts in this tab + dispatch(onRefresh(chartIds, true, 0, dashboardInfo.id)); + }, 100); + } + } + } + }, [ + props.isComponentVisible, + props.renderType, + props.id, + lastRefreshTime, + tabActivationTimes, + dashboardLayout, + dashboardInfo.id, + dispatch, + ]); Review Comment: ### Inefficient effect dependency on tabActivationTimes <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? useEffect depends on a whole tabActivationTimes object, causing the effect to re-run frequently even when only a single tab's activation time changes. ###### Why this matters Including the entire tabActivationTimes object in the dependency array can trigger the effect on any update to that object reference, leading to unnecessary computations and possible redundant chart refreshes across tabs, harming performance especially with many tabs or frequent dashboard interactions. ###### Suggested change ∙ *Feature Preview* Refactor to depend on the per-tab activation time value rather than the entire object. For example: ```jsx // Derive a per-tab activation time value const tabActivationTimeForThisTab = useSelector( state => state.dashboardState.tabActivationTimes?.[props.id] || 0 ); useEffect(() => { if (props.renderType === RENDER_TAB_CONTENT && props.isComponentVisible) { // If a refresh occurred while this tab was inactive, refresh now that it's visible if (lastRefreshTime && tabActivationTimeForThisTab && lastRefreshTime > tabActivationTimeForThisTab) { const chartIds = getChartIdsFromComponent(props.id, dashboardLayout); if (chartIds.length > 0) { setTimeout(() => { dispatch(onRefresh(chartIds, true, 0, dashboardInfo.id)); }, 100); } } } }, [ props.isComponentVisible, props.renderType, props.id, lastRefreshTime, tabActivationTimeForThisTab, dashboardLayout, dashboardInfo.id, dispatch ]); ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/98dc88c5-c5cc-42b4-9038-0e2358da4e16/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/98dc88c5-c5cc-42b4-9038-0e2358da4e16?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/98dc88c5-c5cc-42b4-9038-0e2358da4e16?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/98dc88c5-c5cc-42b4-9038-0e2358da4e16?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/98dc88c5-c5cc-42b4-9038-0e2358da4e16) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:6400e8d7-4293-4f95-8054-225e255ecb2d --> [](6400e8d7-4293-4f95-8054-225e255ecb2d) ########## superset-frontend/src/dashboard/components/gridComponents/Tab/Tab.jsx: ########## @@ -113,6 +114,49 @@ const renderDraggableContent = dropProps => const Tab = props => { const dispatch = useDispatch(); const canEdit = useSelector(state => state.dashboardInfo.dash_edit_perm); + const dashboardLayout = useSelector(state => state.dashboardLayout.present); + const lastRefreshTime = useSelector( + state => state.dashboardState.lastRefreshTime, + ); + const tabActivationTimes = useSelector( + state => state.dashboardState.tabActivationTimes || {}, + ); + const dashboardInfo = useSelector(state => state.dashboardInfo); Review Comment: ### Component with Mixed Responsibilities <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The Tab component has too many responsibilities, handling both UI rendering and complex tab refresh logic. ###### Why this matters Violates the Single Responsibility Principle (SRP). This mixing of concerns makes the component harder to maintain, test, and reuse. ###### Suggested change ∙ *Feature Preview* Extract the tab refresh logic into a custom hook: ```javascript const useTabRefresh = (tabId, isVisible, renderType) => { const dispatch = useDispatch(); const dashboardLayout = useSelector(state => state.dashboardLayout.present); const lastRefreshTime = useSelector(state => state.dashboardState.lastRefreshTime); const tabActivationTimes = useSelector(state => state.dashboardState.tabActivationTimes || {}); const dashboardInfo = useSelector(state => state.dashboardInfo); useEffect(() => { // Tab refresh logic here }, [isVisible, renderType, tabId, lastRefreshTime, tabActivationTimes, dashboardLayout]); }; ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bb7b5254-e81b-494e-8439-54986c816f78/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bb7b5254-e81b-494e-8439-54986c816f78?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bb7b5254-e81b-494e-8439-54986c816f78?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bb7b5254-e81b-494e-8439-54986c816f78?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bb7b5254-e81b-494e-8439-54986c816f78) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:391f3b0b-903a-467d-87b8-ef2566fcbd43 --> [](391f3b0b-903a-467d-87b8-ef2566fcbd43) -- 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]
