codeant-ai-for-open-source[bot] commented on code in PR #37459: URL: https://github.com/apache/superset/pull/37459#discussion_r2739268605
########## superset-frontend/src/dashboard/components/AutoRefreshStatus/StatusIndicatorDot.tsx: ########## @@ -0,0 +1,168 @@ +/** + * 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 { FC, useMemo, useRef, useEffect, useState } from 'react'; +import { css, useTheme } from '@apache-superset/core/ui'; +import { AutoRefreshStatus } from '../../types/autoRefresh'; + +export interface StatusIndicatorDotProps { + /** Current status to display */ + status: AutoRefreshStatus; + /** Size of the dot in pixels */ + size?: number; +} + +/** + * Status indicator configuration mapping. + * + * - Green dot: Refreshed on schedule + * - Blue dot: Fetching data or waiting for first refresh + * - Yellow/warning dot: Delayed + * - Red dot: Error + * - White dot: Paused + */ +interface StatusConfig { + color: string; + needsBorder: boolean; +} + +const getStatusConfig = ( + theme: ReturnType<typeof useTheme>, + status: AutoRefreshStatus, +): StatusConfig => { + switch (status) { + case AutoRefreshStatus.Success: + return { + color: theme.colorSuccess, + needsBorder: false, + }; + case AutoRefreshStatus.Idle: + return { + color: theme.colorInfo, + needsBorder: false, + }; + case AutoRefreshStatus.Fetching: + return { + color: theme.colorInfo, + needsBorder: false, + }; + case AutoRefreshStatus.Delayed: + return { + color: theme.colorWarning, + needsBorder: false, + }; + case AutoRefreshStatus.Error: + return { + color: theme.colorError, + needsBorder: false, + }; + case AutoRefreshStatus.Paused: + return { + color: theme.colorBgContainer, + needsBorder: true, + }; + default: + return { + color: theme.colorTextSecondary, + needsBorder: false, + }; + } +}; + +/** + * A colored dot indicator that shows the auto-refresh status. + * + * Uses CSS transitions to prevent flickering between states. + * The color change is animated smoothly rather than instantly. + */ +export const StatusIndicatorDot: FC<StatusIndicatorDotProps> = ({ + status, + size = 10, +}) => { + const theme = useTheme(); + + // Debounce rapid status changes to prevent flickering + const [displayStatus, setDisplayStatus] = useState(status); + const timerRef = useRef<ReturnType<typeof setTimeout> | null>(null); + + useEffect(() => { + // Clear any pending timer + if (timerRef.current) { + clearTimeout(timerRef.current); + } + + // For fetching state, update immediately to show user something is happening + if (status === AutoRefreshStatus.Fetching) { + setDisplayStatus(status); + } else { + // For other states, debounce to prevent flickering + timerRef.current = setTimeout(() => { + setDisplayStatus(status); + }, 100); + } + + return () => { + if (timerRef.current) { + clearTimeout(timerRef.current); + } + }; + }, [status]); + + const statusConfig = useMemo( + () => getStatusConfig(theme, displayStatus), + [theme, displayStatus], + ); + + const dotStyles = useMemo( + () => css` + display: inline-flex; + align-items: center; + justify-content: center; + width: ${size}px; + height: ${size}px; + border-radius: 50%; + background-color: ${statusConfig.color}; + transition: + background-color ${theme.motionDurationMid} ease-in-out, + border-color ${theme.motionDurationMid} ease-in-out; + border: ${statusConfig.needsBorder + ? `1px solid ${theme.colorBorder}` + : 'none'}; + box-shadow: ${statusConfig.needsBorder + ? 'none' + : `0 0 0 2px ${theme.colorBgContainer}`}; + margin-left: ${theme.marginXS}px; + margin-right: ${theme.marginXS}px; + cursor: help; + `, + [statusConfig, size, theme], + ); + + return ( + <span + css={dotStyles} + role="status" + aria-label={`Auto-refresh status: ${displayStatus}`} + data-test="status-indicator-dot" Review Comment: **Suggestion:** `aria-label` and `data-status` interpolate `displayStatus` directly; if `AutoRefreshStatus` is a numeric enum this will produce numbers instead of readable labels for accessibility and debugging — map statuses to human-readable strings and use that label in `aria-label` and `data-status`. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Screen readers announce numeric enum values. - ⚠️ DOM data-status shows unreadable numeric values. ``` </details> ```suggestion const statusLabel = useMemo(() => { const labels: Record<AutoRefreshStatus, string> = { [AutoRefreshStatus.Success]: 'Success', [AutoRefreshStatus.Idle]: 'Idle', [AutoRefreshStatus.Fetching]: 'Fetching', [AutoRefreshStatus.Delayed]: 'Delayed', [AutoRefreshStatus.Error]: 'Error', [AutoRefreshStatus.Paused]: 'Paused', }; return labels[displayStatus] ?? String(displayStatus); }, [displayStatus]); return ( <span css={dotStyles} role="status" aria-label={`Auto-refresh status: ${statusLabel}`} data-test="status-indicator-dot" data-status={statusLabel} ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open superset-frontend/src/dashboard/components/AutoRefreshStatus/StatusIndicatorDot.tsx and locate the render block at lines 156-164 where `aria-label` and `data-status` interpolate `displayStatus`. 2. Start the frontend and open a dashboard with auto-refresh enabled (the PR adds this indicator to the dashboard header). Trigger different statuses by causing fetches to run and fail/succeed (the UI sets AutoRefreshStatus values based on refresh outcomes). 3. Inspect the DOM for the status indicator element (`data-test="status-indicator-dot"`) and observe the `aria-label` and `data-status` attributes. If `AutoRefreshStatus` in types/autoRefresh is a numeric enum, these attributes will show numbers (e.g., "Auto-refresh status: 2") rather than readable text. 4. Result: screen readers and DOM debugging tools see numeric values instead of human-friendly labels. Mapping enum values to strings (as proposed) produces accessible, debuggable labels. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/dashboard/components/AutoRefreshStatus/StatusIndicatorDot.tsx **Line:** 156:161 **Comment:** *Possible Bug: `aria-label` and `data-status` interpolate `displayStatus` directly; if `AutoRefreshStatus` is a numeric enum this will produce numbers instead of readable labels for accessibility and debugging — map statuses to human-readable strings and use that label in `aria-label` and `data-status`. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> ########## superset-frontend/src/dashboard/components/AutoRefreshStatus/StatusIndicatorDot.test.tsx: ########## @@ -0,0 +1,113 @@ +/** + * 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 { render, screen, act } from 'spec/helpers/testing-library'; +import { StatusIndicatorDot } from './StatusIndicatorDot'; +import { AutoRefreshStatus } from '../../types/autoRefresh'; + +test('renders with success status', () => { + render(<StatusIndicatorDot status={AutoRefreshStatus.Success} />); + const dot = screen.getByTestId('status-indicator-dot'); + expect(dot).toBeInTheDocument(); + expect(dot).toHaveAttribute('data-status', AutoRefreshStatus.Success); +}); + +test('renders with fetching status', () => { + render(<StatusIndicatorDot status={AutoRefreshStatus.Fetching} />); + const dot = screen.getByTestId('status-indicator-dot'); + expect(dot).toHaveAttribute('data-status', AutoRefreshStatus.Fetching); +}); + +test('renders with delayed status', () => { + render(<StatusIndicatorDot status={AutoRefreshStatus.Delayed} />); + const dot = screen.getByTestId('status-indicator-dot'); + expect(dot).toHaveAttribute('data-status', AutoRefreshStatus.Delayed); +}); + +test('renders with idle status', () => { + render(<StatusIndicatorDot status={AutoRefreshStatus.Idle} />); + const dot = screen.getByTestId('status-indicator-dot'); + expect(dot).toHaveAttribute('data-status', AutoRefreshStatus.Idle); +}); + +test('renders with error status', () => { + render(<StatusIndicatorDot status={AutoRefreshStatus.Error} />); + const dot = screen.getByTestId('status-indicator-dot'); + expect(dot).toHaveAttribute('data-status', AutoRefreshStatus.Error); +}); + +test('renders with paused status', () => { + render(<StatusIndicatorDot status={AutoRefreshStatus.Paused} />); + const dot = screen.getByTestId('status-indicator-dot'); + expect(dot).toHaveAttribute('data-status', AutoRefreshStatus.Paused); +}); + +test('has correct accessibility attributes', () => { + render(<StatusIndicatorDot status={AutoRefreshStatus.Success} />); + const dot = screen.getByTestId('status-indicator-dot'); + expect(dot).toHaveAttribute('role', 'status'); + expect(dot).toHaveAttribute('aria-label', 'Auto-refresh status: success'); +}); + +test('fetching status updates immediately without debounce', async () => { + jest.useFakeTimers(); + + const { rerender } = render( + <StatusIndicatorDot status={AutoRefreshStatus.Success} />, + ); + + // Change to fetching - should be immediate + rerender(<StatusIndicatorDot status={AutoRefreshStatus.Fetching} />); + + const dot = screen.getByTestId('status-indicator-dot'); + expect(dot).toHaveAttribute('data-status', AutoRefreshStatus.Fetching); + + jest.useRealTimers(); +}); + +test('debounces non-fetching status changes to prevent flickering', async () => { + jest.useFakeTimers(); + + const { rerender } = render( + <StatusIndicatorDot status={AutoRefreshStatus.Success} />, + ); + + // Change to error - should be debounced + rerender(<StatusIndicatorDot status={AutoRefreshStatus.Error} />); + + // Status should still show success (debounced) + let dot = screen.getByTestId('status-indicator-dot'); + expect(dot).toHaveAttribute('data-status', AutoRefreshStatus.Success); + + // Fast forward past debounce time (100ms) + act(() => { + jest.advanceTimersByTime(150); + }); + + // Now should be error + dot = screen.getByTestId('status-indicator-dot'); + expect(dot).toHaveAttribute('data-status', AutoRefreshStatus.Error); + + jest.useRealTimers(); Review Comment: **Suggestion:** Another fake-timer test also calls `jest.useRealTimers()` only at the end; if an assertion throws the timers aren't restored. Wrap the test body in try/finally and restore timers in finally. Also remove the unnecessary `async` declaration since the test doesn't await anything. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ AutoRefreshStatus debounce test becomes flaky. - ❌ Neighboring timer tests can fail unexpectedly. - ⚠️ Test authoring may hide real failures. ``` </details> ```suggestion test('debounces non-fetching status changes to prevent flickering', () => { jest.useFakeTimers(); try { const { rerender } = render( <StatusIndicatorDot status={AutoRefreshStatus.Success} />, ); // Change to error - should be debounced rerender(<StatusIndicatorDot status={AutoRefreshStatus.Error} />); // Status should still show success (debounced) let dot = screen.getByTestId('status-indicator-dot'); expect(dot).toHaveAttribute('data-status', AutoRefreshStatus.Success); // Fast forward past debounce time (100ms) act(() => { jest.advanceTimersByTime(150); }); // Now should be error dot = screen.getByTestId('status-indicator-dot'); expect(dot).toHaveAttribute('data-status', AutoRefreshStatus.Error); } finally { jest.useRealTimers(); } ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open superset-frontend/src/dashboard/components/AutoRefreshStatus/StatusIndicatorDot.test.tsx and locate the debounce test starting at line 83 which calls jest.useFakeTimers() (line 84) and restores real timers only at line 106. 2. Run the test file with Jest (e.g. yarn test path/to/StatusIndicatorDot.test.tsx). The test renders the component (lines 86-89), rerenders to Error (line 91), asserts the intermediate state (lines 94-95), advances fake timers (lines 98-101), then asserts the final state (lines 103-104). 3. If any assertion (for example the initial expectation at line 95 or final expectation at line 104) throws, execution exits before reaching jest.useRealTimers() at line 106, leaving fake timers active in the Jest process. 4. Observe other tests running after this file failing or behaving differently due to fake timers still active (setTimeout/setInterval controlled by Jest fake timers), reproducing the leak. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/dashboard/components/AutoRefreshStatus/StatusIndicatorDot.test.tsx **Line:** 83:106 **Comment:** *Possible Bug: Another fake-timer test also calls `jest.useRealTimers()` only at the end; if an assertion throws the timers aren't restored. Wrap the test body in try/finally and restore timers in finally. Also remove the unnecessary `async` declaration since the test doesn't await anything. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> ########## superset-frontend/src/dashboard/hooks/useRealTimeDashboard.ts: ########## @@ -0,0 +1,245 @@ +/** + * 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 { useCallback, useMemo } from 'react'; +import { useSelector, useDispatch } from 'react-redux'; +import { AutoRefreshStatus } from '../types/autoRefresh'; +import { DashboardState, RootState } from '../types'; +import { + setAutoRefreshStatus, + setAutoRefreshPaused, + setAutoRefreshPausedByTab, + recordAutoRefreshSuccess, + recordAutoRefreshError, + setAutoRefreshFetchStartTime, + setAutoRefreshPauseOnInactiveTab, +} from '../actions/autoRefresh'; + +type DashboardStateRoot = { + dashboardState: Partial<DashboardState>; +}; + +/** + * Selector: Determines if this is a "real-time" dashboard. + * A dashboard is real-time if it has an auto-refresh frequency > 0. + */ +export const selectIsRealTimeDashboard = (state: DashboardStateRoot): boolean => + (state.dashboardState?.refreshFrequency ?? 0) > 0; + +/** + * Selector: Determines if auto-refresh is manually paused (by user action). + * Does NOT include tab visibility pause. + */ +export const selectIsManuallyPaused = (state: DashboardStateRoot): boolean => Review Comment: **Suggestion:** The `selectIsManuallyPaused` selector expects `DashboardStateRoot` while `useSelector` will call it with the full `RootState`; this mismatched parameter type can lead to incorrect type checking and maintenance issues—use `RootState` to reflect the actual shape provided by `useSelector`. [type error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ useSelector calls in hook show type mismatch. - ⚠️ IDE highlights selector declaration errors. - ❌ CI typecheck can fail on PRs. ``` </details> ```suggestion export const selectIsManuallyPaused = (state: RootState): boolean => ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Inspect superset-frontend/src/dashboard/hooks/useRealTimeDashboard.ts: selector `selectIsManuallyPaused` is declared at lines 48-49 with parameter `DashboardStateRoot`. 2. The selector is consumed in this same hook at line 112: `const isManuallyPaused = useSelector(selectIsManuallyPaused);` where `useSelector` provides the global `RootState`. 3. Run the project's TypeScript type-check (or open in IDE): the selector's parameter type (`DashboardStateRoot`) is narrower than the `RootState` provided by `useSelector`, causing a type incompatibility reported at the call site (line 112) and/or declaration (lines 48-49). 4. Updating the selector signature to accept `RootState` (as proposed) resolves the mismatch and clears the type-checker/IDE warning. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/dashboard/hooks/useRealTimeDashboard.ts **Line:** 48:48 **Comment:** *Type Error: The `selectIsManuallyPaused` selector expects `DashboardStateRoot` while `useSelector` will call it with the full `RootState`; this mismatched parameter type can lead to incorrect type checking and maintenance issues—use `RootState` to reflect the actual shape provided by `useSelector`. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> ########## superset-frontend/src/dashboard/components/FiltersBadge/index.tsx: ########## @@ -106,10 +107,14 @@ const indicatorsInitialState: Indicator[] = []; export const FiltersBadge = ({ chartId }: FiltersBadgeProps) => { const dispatch = useDispatch(); - const datasources = useSelector<RootState, any>(state => state.datasources); - const dashboardFilters = useSelector<RootState, any>( - state => state.dashboardFilters, + const isAutoRefreshing = useIsAutoRefreshing(); + const datasources = useSelector<RootState, RootState['datasources']>( + state => state.datasources, ); + const dashboardFilters = useSelector< + RootState, + RootState['dashboardFilters'] + >(state => state.dashboardFilters); Review Comment: **Suggestion:** `useSelector` is selecting entire `datasources` and `dashboardFilters` objects which will change reference often and trigger unnecessary recomputations; pass `shallowEqual` as the equality function to `useSelector` (or select only the minimal fields needed) to avoid frequent re-renders and expensive recalculations. [performance] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Extra re-renders during dashboard state updates. - ❌ Slower dashboard filter indicator recalculations. ``` </details> ```suggestion shallowEqual, ); const dashboardFilters = useSelector< RootState, RootState['dashboardFilters'] >(state => state.dashboardFilters, shallowEqual); ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open a dashboard which mounts FiltersBadge (file: superset-frontend/src/dashboard/components/FiltersBadge/index.tsx). FiltersBadge selects large slices datasources and dashboardFilters at lines ~111-117. 2. Trigger frequent dashboard state updates (for example auto-refresh / chart state updates). FiltersBadge's effect depends on datasources (effect dependency list includes datasources near the setDashboardIndicators effect at index.tsx:168-176). 3. Because useSelector currently returns the entire datasources and dashboardFilters objects without shallowEqual, any update that replaces those objects by reference (even when contents unchanged) will cause FiltersBadge to re-run effects and recompute indicators via selectIndicatorsForChart at the same effect (setDashboardIndicators is called there). 4. Observe increased recalculation: selectIndicatorsForChart is invoked more often (index.tsx where setDashboardIndicators is called), causing extra CPU work and re-renders of the FiltersBadge UI during normal dashboard refresh cycles. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/dashboard/components/FiltersBadge/index.tsx **Line:** 113:117 **Comment:** *Performance: `useSelector` is selecting entire `datasources` and `dashboardFilters` objects which will change reference often and trigger unnecessary recomputations; pass `shallowEqual` as the equality function to `useSelector` (or select only the minimal fields needed) to avoid frequent re-renders and expensive recalculations. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> ########## superset-frontend/src/dashboard/hooks/useRealTimeDashboard.ts: ########## @@ -0,0 +1,245 @@ +/** + * 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 { useCallback, useMemo } from 'react'; +import { useSelector, useDispatch } from 'react-redux'; +import { AutoRefreshStatus } from '../types/autoRefresh'; +import { DashboardState, RootState } from '../types'; +import { + setAutoRefreshStatus, + setAutoRefreshPaused, + setAutoRefreshPausedByTab, + recordAutoRefreshSuccess, + recordAutoRefreshError, + setAutoRefreshFetchStartTime, + setAutoRefreshPauseOnInactiveTab, +} from '../actions/autoRefresh'; + +type DashboardStateRoot = { + dashboardState: Partial<DashboardState>; +}; + +/** + * Selector: Determines if this is a "real-time" dashboard. + * A dashboard is real-time if it has an auto-refresh frequency > 0. + */ +export const selectIsRealTimeDashboard = (state: DashboardStateRoot): boolean => + (state.dashboardState?.refreshFrequency ?? 0) > 0; + +/** + * Selector: Determines if auto-refresh is manually paused (by user action). + * Does NOT include tab visibility pause. + */ +export const selectIsManuallyPaused = (state: DashboardStateRoot): boolean => + state.dashboardState?.autoRefreshPaused === true; + +/** + * Selector: Determines if auto-refresh is paused. + * Paused can be due to manual pause or tab visibility. + */ +export const selectIsPaused = (state: DashboardStateRoot): boolean => Review Comment: **Suggestion:** The `selectIsPaused` selector is declared to accept `DashboardStateRoot` but is invoked via `useSelector` with the full `RootState`; change the parameter to `RootState` to ensure correct typing and prevent incorrect assumptions about state shape. [type error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ useSelector type mismatch in dashboard hook. - ⚠️ Developer workflow interrupted by IDE warnings. - ❌ Potential CI pipeline typecheck failure. ``` </details> ```suggestion export const selectIsPaused = (state: RootState): boolean => ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open superset-frontend/src/dashboard/hooks/useRealTimeDashboard.ts and find `selectIsPaused` at lines 55-57. 2. Within the same file the selector is invoked via `useSelector` at line 113: `const isPaused = useSelector(selectIsPaused);` which supplies the full `RootState`. 3. Execute TypeScript checking (tsc) or view the file in an editor: the selector signature expecting `DashboardStateRoot` conflicts with `RootState` passed by `useSelector`, generating a type error at the call site (line 113) or the selector declaration (lines 55-57). 4. Change the selector parameter to `RootState` as suggested to align types and eliminate the type-checker errors. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/dashboard/hooks/useRealTimeDashboard.ts **Line:** 55:55 **Comment:** *Type Error: The `selectIsPaused` selector is declared to accept `DashboardStateRoot` but is invoked via `useSelector` with the full `RootState`; change the parameter to `RootState` to ensure correct typing and prevent incorrect assumptions about state shape. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> ########## superset-frontend/src/dashboard/components/AutoRefreshStatus/StatusIndicatorDot.test.tsx: ########## @@ -0,0 +1,113 @@ +/** + * 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 { render, screen, act } from 'spec/helpers/testing-library'; +import { StatusIndicatorDot } from './StatusIndicatorDot'; +import { AutoRefreshStatus } from '../../types/autoRefresh'; + +test('renders with success status', () => { + render(<StatusIndicatorDot status={AutoRefreshStatus.Success} />); + const dot = screen.getByTestId('status-indicator-dot'); + expect(dot).toBeInTheDocument(); + expect(dot).toHaveAttribute('data-status', AutoRefreshStatus.Success); +}); + +test('renders with fetching status', () => { + render(<StatusIndicatorDot status={AutoRefreshStatus.Fetching} />); + const dot = screen.getByTestId('status-indicator-dot'); + expect(dot).toHaveAttribute('data-status', AutoRefreshStatus.Fetching); +}); + +test('renders with delayed status', () => { + render(<StatusIndicatorDot status={AutoRefreshStatus.Delayed} />); + const dot = screen.getByTestId('status-indicator-dot'); + expect(dot).toHaveAttribute('data-status', AutoRefreshStatus.Delayed); +}); + +test('renders with idle status', () => { + render(<StatusIndicatorDot status={AutoRefreshStatus.Idle} />); + const dot = screen.getByTestId('status-indicator-dot'); + expect(dot).toHaveAttribute('data-status', AutoRefreshStatus.Idle); +}); + +test('renders with error status', () => { + render(<StatusIndicatorDot status={AutoRefreshStatus.Error} />); + const dot = screen.getByTestId('status-indicator-dot'); + expect(dot).toHaveAttribute('data-status', AutoRefreshStatus.Error); +}); + +test('renders with paused status', () => { + render(<StatusIndicatorDot status={AutoRefreshStatus.Paused} />); + const dot = screen.getByTestId('status-indicator-dot'); + expect(dot).toHaveAttribute('data-status', AutoRefreshStatus.Paused); +}); + +test('has correct accessibility attributes', () => { + render(<StatusIndicatorDot status={AutoRefreshStatus.Success} />); + const dot = screen.getByTestId('status-indicator-dot'); + expect(dot).toHaveAttribute('role', 'status'); + expect(dot).toHaveAttribute('aria-label', 'Auto-refresh status: success'); +}); + +test('fetching status updates immediately without debounce', async () => { + jest.useFakeTimers(); + + const { rerender } = render( + <StatusIndicatorDot status={AutoRefreshStatus.Success} />, + ); + + // Change to fetching - should be immediate + rerender(<StatusIndicatorDot status={AutoRefreshStatus.Fetching} />); + + const dot = screen.getByTestId('status-indicator-dot'); + expect(dot).toHaveAttribute('data-status', AutoRefreshStatus.Fetching); + + jest.useRealTimers(); Review Comment: **Suggestion:** Tests use fake timers but call `jest.useRealTimers()` only at the end of the test body; if the test throws before that call the real timers won't be restored and can leak to other tests. Wrap fake-timer tests in try/finally and ensure timers are always restored. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ AutoRefreshStatus unit tests become flaky. - ❌ Timer-dependent tests fail unpredictably. - ⚠️ CI test runs may intermittently fail. ``` </details> ```suggestion test('fetching status updates immediately without debounce', () => { jest.useFakeTimers(); try { const { rerender } = render( <StatusIndicatorDot status={AutoRefreshStatus.Success} />, ); // Change to fetching - should be immediate rerender(<StatusIndicatorDot status={AutoRefreshStatus.Fetching} />); const dot = screen.getByTestId('status-indicator-dot'); expect(dot).toHaveAttribute('data-status', AutoRefreshStatus.Fetching); } finally { jest.useRealTimers(); } ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open file superset-frontend/src/dashboard/components/AutoRefreshStatus/StatusIndicatorDot.test.tsx and locate the test beginning at line 67 which calls jest.useFakeTimers() (line 67) and only calls jest.useRealTimers() at line 80. 2. Run the specific test file with Jest (e.g. yarn test superset-frontend/src/dashboard/components/AutoRefreshStatus/StatusIndicatorDot.test.tsx). The test executes the code path at lines 70-76: render, rerender to Fetching (lines 71-75), then calls screen.getByTestId() at line 77 and an assertion at line 78. 3. If the assertion at line 78 throws (for example the element is not found or the attribute differs), the test will exit before reaching jest.useRealTimers() at line 80, leaving Jest running with fake timers enabled. 4. Observe subsequent tests in the same Jest process (other files or later tests) misbehave: timer-based APIs (setTimeout, setInterval) will be controlled by fake timers causing unexpected timing and flaky failures across the suite. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/dashboard/components/AutoRefreshStatus/StatusIndicatorDot.test.tsx **Line:** 67:80 **Comment:** *Possible Bug: Tests use fake timers but call `jest.useRealTimers()` only at the end of the test body; if the test throws before that call the real timers won't be restored and can leak to other tests. Wrap fake-timer tests in try/finally and ensure timers are always restored. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> ########## superset-frontend/src/dashboard/hooks/useRealTimeDashboard.ts: ########## @@ -0,0 +1,245 @@ +/** + * 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 { useCallback, useMemo } from 'react'; +import { useSelector, useDispatch } from 'react-redux'; +import { AutoRefreshStatus } from '../types/autoRefresh'; +import { DashboardState, RootState } from '../types'; +import { + setAutoRefreshStatus, + setAutoRefreshPaused, + setAutoRefreshPausedByTab, + recordAutoRefreshSuccess, + recordAutoRefreshError, + setAutoRefreshFetchStartTime, + setAutoRefreshPauseOnInactiveTab, +} from '../actions/autoRefresh'; + +type DashboardStateRoot = { + dashboardState: Partial<DashboardState>; +}; + +/** + * Selector: Determines if this is a "real-time" dashboard. + * A dashboard is real-time if it has an auto-refresh frequency > 0. + */ +export const selectIsRealTimeDashboard = (state: DashboardStateRoot): boolean => Review Comment: **Suggestion:** The selector is typed to accept `DashboardStateRoot` but is used with `useSelector`, which supplies the full `RootState`; this causes a TypeScript typing mismatch and can hide bugs or cause incorrect assumptions when the actual root state shape differs. Change the selector parameter type to `RootState` to match `useSelector` usage. [type error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ TypeScript errors in useRealTimeDashboard hook. - ⚠️ Editor/IDE reports selector type mismatch. - ❌ CI typecheck may fail blocking merges. ``` </details> ```suggestion export const selectIsRealTimeDashboard = (state: RootState): boolean => ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open file superset-frontend/src/dashboard/hooks/useRealTimeDashboard.ts and view the selector declaration at lines 41-42 (`selectIsRealTimeDashboard`). 2. In the same file, observe the call site at line 111: `const isRealTimeDashboard = useSelector(selectIsRealTimeDashboard);` — `useSelector` passes the app RootState (see lines 111-114 where other selectors are also passed to `useSelector`). 3. Run TypeScript type-check (e.g., npm run typecheck or tsc) or view in IDE: the selector signature expecting `DashboardStateRoot` is incompatible with the `RootState` provided by `useSelector`, producing a type error at the call site (`useSelector(selectIsRealTimeDashboard)`). 4. Result: editor/CI reports a type-mismatch warning/error pointing to the selector declaration (line 41-42) and the call site (line 111). Changing the selector parameter to `RootState` (as proposed) aligns the types and removes the type-check error. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/dashboard/hooks/useRealTimeDashboard.ts **Line:** 41:41 **Comment:** *Type Error: The selector is typed to accept `DashboardStateRoot` but is used with `useSelector`, which supplies the full `RootState`; this causes a TypeScript typing mismatch and can hide bugs or cause incorrect assumptions when the actual root state shape differs. Change the selector parameter type to `RootState` to match `useSelector` usage. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> ########## superset-frontend/src/dashboard/hooks/useRealTimeDashboard.ts: ########## @@ -0,0 +1,245 @@ +/** + * 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 { useCallback, useMemo } from 'react'; +import { useSelector, useDispatch } from 'react-redux'; +import { AutoRefreshStatus } from '../types/autoRefresh'; +import { DashboardState, RootState } from '../types'; +import { + setAutoRefreshStatus, + setAutoRefreshPaused, + setAutoRefreshPausedByTab, + recordAutoRefreshSuccess, + recordAutoRefreshError, + setAutoRefreshFetchStartTime, + setAutoRefreshPauseOnInactiveTab, +} from '../actions/autoRefresh'; + +type DashboardStateRoot = { + dashboardState: Partial<DashboardState>; +}; + +/** + * Selector: Determines if this is a "real-time" dashboard. + * A dashboard is real-time if it has an auto-refresh frequency > 0. + */ +export const selectIsRealTimeDashboard = (state: DashboardStateRoot): boolean => + (state.dashboardState?.refreshFrequency ?? 0) > 0; + +/** + * Selector: Determines if auto-refresh is manually paused (by user action). + * Does NOT include tab visibility pause. + */ +export const selectIsManuallyPaused = (state: DashboardStateRoot): boolean => + state.dashboardState?.autoRefreshPaused === true; + +/** + * Selector: Determines if auto-refresh is paused. + * Paused can be due to manual pause or tab visibility. + */ +export const selectIsPaused = (state: DashboardStateRoot): boolean => + state.dashboardState?.autoRefreshPaused === true || + state.dashboardState?.autoRefreshPausedByTab === true; + +/** + * Selector: Computes the effective refresh status for the indicator. + * + * Priority order: + * 1. If not a real-time dashboard → Idle + * 2. If paused (manually or by tab) → Paused + * 3. If fetching → Fetching + * 4. If refreshErrorCount >= 2 → Error + * 5. If refreshErrorCount === 1 → Delayed + * 6. Otherwise → Current status from state + */ +export const selectEffectiveRefreshStatus = ( + state: DashboardStateRoot, Review Comment: **Suggestion:** `selectEffectiveRefreshStatus` is declared to accept `DashboardStateRoot` but is used with `useSelector`, which passes the full `RootState`; this type mismatch can hide incorrect assumptions about the root state structure—update the selector signature to `RootState`. [type error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Type-check errors for selectEffectiveRefreshStatus. - ⚠️ IDE highlights mismatch at useSelector call (line 114). - ❌ Failing CI typecheck blocks merges. ``` </details> ```suggestion state: RootState, ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open superset-frontend/src/dashboard/hooks/useRealTimeDashboard.ts and locate `selectEffectiveRefreshStatus` declared at lines 70-105. 2. Confirm the selector is consumed in this file at line 114: `const effectiveStatus = useSelector(selectEffectiveRefreshStatus);` where `useSelector` provides the global `RootState`. 3. Run TypeScript type-check (`npm run typecheck`/tsc) or check in IDE: the selector signature expecting `DashboardStateRoot` is not the same type `useSelector` supplies (`RootState`), causing a type error flagged at the call site (line 114) and/or declaration (lines 70-105). 4. Update the selector signature to accept `RootState` (as proposed) to align types and clear type-checker/CI/IDE errors. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/dashboard/hooks/useRealTimeDashboard.ts **Line:** 71:71 **Comment:** *Type Error: `selectEffectiveRefreshStatus` is declared to accept `DashboardStateRoot` but is used with `useSelector`, which passes the full `RootState`; this type mismatch can hide incorrect assumptions about the root state structure—update the selector signature to `RootState`. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> ########## superset-frontend/src/dashboard/components/Header/useHeaderActionsDropdownMenu.tsx: ########## @@ -329,5 +339,19 @@ export const useHeaderActionsMenu = ({ userCanShare, ]); - return [menu, isDropdownVisible, setIsDropdownVisible]; + if (!menuRef.current) { + menuRef.current = menu; + } + + useEffect(() => { + if (!isDropdownVisible) { + menuRef.current = menu; + } + }, [isDropdownVisible, menu]); + + return [ + isDropdownVisible && menuRef.current ? menuRef.current : menu, Review Comment: **Suggestion:** Caching the rendered React element (`menu`) in `menuRef.current` and returning that cached element while the dropdown is visible preserves the element instance (and therefore its props/handlers) across renders; this causes stale event handlers and props to be used when `handleMenuClick` or other dependencies change while the dropdown is open. Return the current `menu` element instead of the cached element to ensure handlers and props are up-to-date. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Header actions menu can invoke outdated click handlers. - ⚠️ Menu props updates not reflected while dropdown open. - ⚠️ User actions may run stale callbacks. ``` </details> ```suggestion menu, ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. A component renders and uses `useHeaderActionsMenu` from superset-frontend/src/dashboard/components/Header/useHeaderActionsDropdownMenu.tsx; the hook computes `menu` in the useMemo block and stores it into `menuRef.current` on first render (see lines near 342-344). 2. The dropdown is opened (the consumer sets `isDropdownVisible` true), so the hook's return uses the cached element at lines 352-353: `menuRef.current` is returned while visible. 3. While the dropdown remains open, one of the hook's dependencies changes (for example `isLoading`, `dashboardTitle`, or any dependency listed in the useMemo/useCallback dependency lists around lines ~117-129 and the useMemo dependencies). The hook creates a new `menu` instance with updated props/handlers. 4. Because the hook continues returning the cached `menuRef.current`, the visible dropdown retains the old React element and its old handlers, causing stale handlers/props to be used until the dropdown is closed and reopened. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/dashboard/components/Header/useHeaderActionsDropdownMenu.tsx **Line:** 353:353 **Comment:** *Logic Error: Caching the rendered React element (`menu`) in `menuRef.current` and returning that cached element while the dropdown is visible preserves the element instance (and therefore its props/handlers) across renders; this causes stale event handlers and props to be used when `handleMenuClick` or other dependencies change while the dropdown is open. Return the current `menu` element instead of the cached element to ensure handlers and props are up-to-date. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> ########## superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.jsx: ########## @@ -597,18 +608,18 @@ const Chart = props => { }, [exportTable]); const forceRefresh = useCallback(() => { - boundActionCreators.logEvent(LOG_ACTIONS_FORCE_REFRESH_CHART, { + logEventAction(LOG_ACTIONS_FORCE_REFRESH_CHART, { slice_id: slice.slice_id, is_cached: isCached, }); - return boundActionCreators.refreshChart(chart.id, true, props.dashboardId); + return refreshChartAction(chart.id, true, dashboardId); }, [ - boundActionCreators.refreshChart, + refreshChartAction, chart.id, Review Comment: **Suggestion:** `forceRefresh` calls `refreshChartAction(chart.id, ...)` while `chart.id` may be undefined when the chart fallback object is used; this can result in dispatching a refresh for an undefined id — use the `chartId` prop to reliably identify the chart and update the dependency array accordingly. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Force-refresh could dispatch wrong chart id. - ⚠️ Affects manual refresh in SliceHeader component. - ⚠️ Limited scope; chart objects usually include id. ``` </details> ```suggestion return refreshChartAction(chartId, true, dashboardId); }, [ refreshChartAction, chartId, ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Inspect Chart.jsx: the chart selector is `const chart = useSelector(state => state.charts[chartId] || EMPTY_OBJECT);` (hunk around line 196). `chartId` is the stable prop passed into the component (props destructured at top of Chart.jsx). 2. The forceRefresh callback is defined at Chart.jsx lines 610-623 (existing snippet). It calls `refreshChartAction(chart.id, true, dashboardId)` (line 615 in the hunk). 3. SliceHeader receives `forceRefresh={forceRefresh}` (see SliceHeader prop wiring in the render block around line ~647). A user clicking the manual refresh control will execute this callback. 4. If the `chart` object is present but missing `id`, `refreshChartAction` will be invoked with `undefined` as the chart id (line 615), potentially dispatching an invalid refresh. In practice this is unlikely because chart objects normally include id and the component returns early when chart is exactly EMPTY_OBJECT; therefore the condition where chart exists but lacks `id` is rare. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.jsx **Line:** 615:618 **Comment:** *Possible Bug: `forceRefresh` calls `refreshChartAction(chart.id, ...)` while `chart.id` may be undefined when the chart fallback object is used; this can result in dispatching a refresh for an undefined id — use the `chartId` prop to reliably identify the chart and update the dependency array accordingly. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> -- 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]
