codeant-ai-for-open-source[bot] commented on code in PR #36936:
URL: https://github.com/apache/superset/pull/36936#discussion_r2666834866
##########
superset-frontend/packages/superset-ui-core/src/components/Timer/index.tsx:
##########
@@ -35,7 +35,9 @@ export function Timer({
status = 'success',
}: TimerProps) {
const theme = useTheme();
- const [clockStr, setClockStr] = useState('00:00:00.00');
+ const [clockStr, setClockStr] = useState(
+ startTime && endTime ? fDuration(startTime, endTime) : '00:00:00.00',
Review Comment:
**Suggestion:** The fallback string uses two decimal places ('00:00:00.00')
while the `fDuration` formatting uses milliseconds with three digits ('.SSS');
this produces inconsistent display and may confuse consumers. Change the
fallback to match the same millisecond precision. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
startTime && endTime ? fDuration(startTime, endTime) : '00:00:00.000',
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The fallback uses two decimal places while `fDuration` formats milliseconds
with three digits ('.SSS'). Making the fallback '00:00:00.000' keeps the UI
consistent and avoids surprising format changes when the value is derived from
`fDuration`.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/packages/superset-ui-core/src/components/Timer/index.tsx
**Line:** 39:39
**Comment:**
*Possible Bug: The fallback string uses two decimal places
('00:00:00.00') while the `fDuration` formatting uses milliseconds with three
digits ('.SSS'); this produces inconsistent display and may confuse consumers.
Change the fallback to match the same millisecond precision.
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/SqlLab/components/QueryStatusBar/QueryStatusBar.test.tsx:
##########
@@ -0,0 +1,161 @@
+/**
+ * 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 { isValidElement } from 'react';
+import { render, screen } from 'spec/helpers/testing-library';
+import { QueryState, type QueryResponse } from '@superset-ui/core';
+import QueryStatusBar from '.';
+
+jest.mock('../QueryStateLabel', () => ({
+ __esModule: true,
+ default: ({ query }: { query: { state: QueryState } }) => (
+ <div data-test="query-state-label">{query.state}</div>
+ ),
+}));
+
+const createMockQuery = (
+ overrides: Partial<QueryResponse> = {},
+): QueryResponse =>
+ ({
+ id: 'test-query-id',
+ dbId: 1,
+ sql: 'SELECT * FROM test',
+ sqlEditorId: 'test-editor',
+ tab: 'Test Tab',
+ ctas: false,
+ cached: false,
+ progress: 0,
+ startDttm: Date.now() - 1000,
+ endDttm: undefined,
+ state: QueryState.Running,
+ tempSchema: null,
+ tempTable: null,
+ userId: 1,
+ executedSql: null,
+ rows: 0,
+ queryLimit: 100,
+ catalog: null,
+ schema: 'test_schema',
+ errorMessage: null,
+ extra: {},
+ results: undefined,
+ ...overrides,
+ }) as QueryResponse;
+
+test('is valid element', () => {
+ const query = createMockQuery();
+ expect(isValidElement(<QueryStatusBar query={query} />)).toBe(true);
+});
+
+test('renders query state label', () => {
+ const query = createMockQuery({ state: QueryState.Running });
+ render(<QueryStatusBar query={query} />);
+ expect(screen.getByTestId('query-state-label')).toBeInTheDocument();
+ expect(screen.getByText('Query State:')).toBeInTheDocument();
+});
+
+test('renders elapsed time section', () => {
+ const query = createMockQuery({ state: QueryState.Running });
+ render(<QueryStatusBar query={query} />);
+ expect(screen.getByText('Elapsed:')).toBeInTheDocument();
+});
+
+test('renders steps for running query', () => {
+ const query = createMockQuery({ state: QueryState.Running, progress: 50 });
+ render(<QueryStatusBar query={query} />);
+ expect(screen.getByText('Validate query')).toBeInTheDocument();
+ expect(screen.getByText('Connect to engine')).toBeInTheDocument();
+ expect(screen.getByText('Running')).toBeInTheDocument();
+ expect(screen.getByText('Download to client')).toBeInTheDocument();
+ expect(screen.getByText('Finish')).toBeInTheDocument();
+});
+
+test('renders steps for pending query', () => {
+ const query = createMockQuery({ state: QueryState.Pending });
+ render(<QueryStatusBar query={query} />);
+ expect(screen.getByText('Validate query')).toBeInTheDocument();
+});
+
+test('returns null when query is successful with results', () => {
+ const query = createMockQuery({
+ state: QueryState.Success,
+ results: {
+ displayLimitReached: false,
+ columns: [],
+ selected_columns: [],
+ expanded_columns: [],
+ data: [],
+ query: { limit: 100 },
+ },
+ });
+ const { container } = render(<QueryStatusBar query={query} />);
+ expect(container.firstChild).toBeEmptyDOMElement();
Review Comment:
**Suggestion:** The test asserts
`expect(container.firstChild).toBeEmptyDOMElement()` when the component
intentionally returns `null` for successful queries with results;
`toBeEmptyDOMElement` expects an Element and will throw if `firstChild` is
`null`. Replace the assertion with `toBeNull()` to correctly assert the
component rendered nothing. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
expect(container.firstChild).toBeNull();
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The intent is to assert that the component rendered nothing.
container.firstChild can be null if the component returns null,
and toBeEmptyDOMElement expects an Element (it will throw/assert incorrectly
when given null). Replacing with toBeNull()
correctly expresses the intent and avoids false failures/exceptions in the
test.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/SqlLab/components/QueryStatusBar/QueryStatusBar.test.tsx
**Line:** 107:107
**Comment:**
*Logic Error: The test asserts
`expect(container.firstChild).toBeEmptyDOMElement()` when the component
intentionally returns `null` for successful queries with results;
`toBeEmptyDOMElement` expects an Element and will throw if `firstChild` is
`null`. Replace the assertion with `toBeNull()` to correctly assert the
component rendered nothing.
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/SqlLab/components/SouthPane/Results.tsx:
##########
@@ -71,30 +76,32 @@ const Results: FC<Props> = ({
);
}
- if (
+ const hasNoStoredResults =
isFeatureEnabled(FeatureFlag.SqllabBackendPersistence) &&
latestQuery.state === 'success' &&
!latestQuery.resultsKey &&
- !latestQuery.results
- ) {
- return (
- <Alert
- type="warning"
- message={t('No stored results found, you need to re-run your query')}
- />
- );
- }
+ !latestQuery.results;
return (
- <ResultSet
- search
- queryId={latestQuery.id}
- database={databases[latestQuery.dbId]}
- displayLimit={displayLimit}
- defaultQueryLimit={defaultQueryLimit}
- showSql
- showSqlInline
- />
+ <>
+ <QueryStatusBar key={latestQueryId} query={latestQuery} />
+ {hasNoStoredResults ? (
+ <Alert
+ type="warning"
+ message={t('No stored results found, you need to re-run your query')}
+ />
+ ) : (
+ <ResultSet
+ search
+ queryId={latestQuery.id}
+ database={databases[latestQuery.dbId]}
Review Comment:
**Suggestion:** Accessing `databases[latestQuery.dbId]` can throw if
`databases` is undefined; guard the access so the prop passed to `ResultSet` is
undefined when `databases` is missing, preventing a runtime exception. [null
pointer]
**Severity Level:** Minor ⚠️
```suggestion
database={databases ? databases[latestQuery.dbId] : undefined}
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Also a reasonable defensive change. If `databases` can be undefined
transiently, indexing into it will throw. Guarding the access (or using
optional chaining) is harmless and prevents a crash. If the app invariant
guarantees `databases` exists, it's unnecessary but not harmful.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/SqlLab/components/SouthPane/Results.tsx
**Line:** 97:97
**Comment:**
*Null Pointer: Accessing `databases[latestQuery.dbId]` can throw if
`databases` is undefined; guard the access so the prop passed to `ResultSet` is
undefined when `databases` is missing, preventing a runtime exception.
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/SqlLab/components/QueryStatusBar/QueryStatusBar.test.tsx:
##########
@@ -0,0 +1,161 @@
+/**
+ * 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 { isValidElement } from 'react';
+import { render, screen } from 'spec/helpers/testing-library';
+import { QueryState, type QueryResponse } from '@superset-ui/core';
+import QueryStatusBar from '.';
+
+jest.mock('../QueryStateLabel', () => ({
+ __esModule: true,
+ default: ({ query }: { query: { state: QueryState } }) => (
+ <div data-test="query-state-label">{query.state}</div>
Review Comment:
**Suggestion:** The mocked `QueryStateLabel` component sets a `data-test`
attribute but the tests call `getByTestId(...)` which queries `data-testid`.
This mismatch will cause the `getByTestId('query-state-label')` assertions to
fail because the mock doesn't expose `data-testid`. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
<div data-testid="query-state-label">{query.state}</div>
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This is a valid, actionable bugfix. The test file queries by test id using
getByTestId('query-state-label'),
which looks for data-testid. The mock currently renders data-test, so the
selector will not find the element and
tests that rely on getByTestId will fail. Switching the mock to data-testid
directly fixes the failing selector
with minimal, localized change.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/SqlLab/components/QueryStatusBar/QueryStatusBar.test.tsx
**Line:** 27:27
**Comment:**
*Possible Bug: The mocked `QueryStateLabel` component sets a
`data-test` attribute but the tests call `getByTestId(...)` which queries
`data-testid`. This mismatch will cause the `getByTestId('query-state-label')`
assertions to fail because the mock doesn't expose `data-testid`.
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/SqlLab/components/SouthPane/Results.tsx:
##########
@@ -52,10 +53,14 @@ const Results: FC<Props> = ({
({ sqlLab: { databases } }: SqlLabRootState) => databases,
shallowEqual,
);
- const latestQuery = useSelector(
- ({ sqlLab: { queries } }: SqlLabRootState) => queries[latestQueryId || ''],
+ const queries = useSelector(
+ ({ sqlLab: { queries } }: SqlLabRootState) => queries,
Review Comment:
**Suggestion:** Indexing into `queries` can throw if `queries` is undefined
(e.g. not yet loaded in state). Ensure the selector returns a default empty
object or guard the lookup so `queries[latestQueryId]` is not evaluated on
undefined. [null pointer]
**Severity Level:** Minor ⚠️
```suggestion
({ sqlLab: { queries } }: SqlLabRootState) => queries ?? {},
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This is a valid defensive improvement. If the Redux slice guarantees
`queries` is always an object, it's redundant — but that's an assumption.
Returning `queries ?? {}` from the selector prevents a potential runtime
TypeError when the state is temporarily undefined (e.g. during hydration). The
proposed change is small, correct, and safe.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/SqlLab/components/SouthPane/Results.tsx
**Line:** 57:57
**Comment:**
*Null Pointer: Indexing into `queries` can throw if `queries` is
undefined (e.g. not yet loaded in state). Ensure the selector returns a default
empty object or guard the lookup so `queries[latestQueryId]` is not evaluated
on undefined.
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/SqlLab/components/ResultSet/index.tsx:
##########
@@ -856,34 +854,20 @@ const ResultSet = ({
}
}
- let progressBar;
- if (query.progress > 0) {
- progressBar = (
- <ProgressBar percent={parseInt(query.progress.toFixed(0), 10)} striped />
- );
- }
-
const progressMsg = query?.extra?.progress ?? null;
return (
- <>
- <ResultlessStyles>
- <div>{!progressBar && <Loading position="normal" />}</div>
- {/* show loading bar whenever progress bar is completed but needs time
to render */}
- <div>{query.progress === 100 && <Loading position="normal" />}</div>
- <QueryStateLabel query={query} />
- <div>
- {progressMsg && <Alert type="success" message={progressMsg} />}
- </div>
- <div>{query.progress !== 100 && progressBar}</div>
- {trackingUrl && <div>{trackingUrl}</div>}
- </ResultlessStyles>
+ <ResultlessStyles>
+ {progressMsg && (
+ <Alert type="success" message={progressMsg} closable={false} />
+ )}
+ {trackingUrl && <div>{trackingUrl}</div>}
Review Comment:
**Suggestion:** Rendering `trackingUrl` wrapped inside an extra `<div>` may
change styling or semantics of the button element stored in `trackingUrl` (it
is a JSX Button element); render the JSX element directly to preserve its
attributes and styling instead of nesting it inside a new container. [possible
bug]
**Severity Level:** Critical 🚨
```suggestion
{trackingUrl && trackingUrl}
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
trackingUrl is a JSX Button element. Wrapping it in an extra div can change
layout/styling and is inconsistent
with other places where trackingUrl is rendered directly. Rendering the
element directly preserves attributes
and styling.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/SqlLab/components/ResultSet/index.tsx
**Line:** 864:864
**Comment:**
*Possible Bug: Rendering `trackingUrl` wrapped inside an extra `<div>`
may change styling or semantics of the button element stored in `trackingUrl`
(it is a JSX Button element); render the JSX element directly to preserve its
attributes and styling instead of nesting it inside a new container.
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/SqlLab/components/QueryStatusBar/index.tsx:
##########
@@ -0,0 +1,216 @@
+/**
+ * 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, createContext, useContext, useRef } from 'react';
+import { styled } from '@apache-superset/core';
+import {
+ Flex,
+ Steps,
+ type StepsProps,
+ StyledSpin,
+ Timer,
+} from '@superset-ui/core/components';
+import { QueryResponse, QueryState, t, usePrevious } from '@superset-ui/core';
+import QueryStateLabel from '../QueryStateLabel';
+
+type QueryStatusBarProps = {
+ query: QueryResponse;
+};
+
+const STATE_TO_STEP: Record<string, number> = {
+ offline: 4,
+ failed: 4,
+ pending: 0,
+ fetching: 3,
+ running: 2,
+ stopped: 4,
+ success: 4,
+};
+
+const ERROR_STATE = [QueryState.Failed, QueryState.Stopped];
+
+const StyledSteps = styled.div`
+ & .ant-steps {
+ margin: ${({ theme }) => theme.sizeUnit * 2}px 0;
+ }
+`;
+
+const ActiveDot = styled.div`
+ position: relative;
+ display: flex;
+ align-items: center;
+ justify-content: center;
+
+ &::before,
+ &::after {
+ content: '';
+ position: absolute;
+ width: 8px;
+ height: 8px;
+ border-radius: 50%;
+ background-color: ${({ theme }) => theme.colorPrimary};
+ top: -1px;
+ opacity: 0;
+ animation: pulse 2s ease-out infinite;
+ }
+
+ &::after {
+ animation-delay: 1s;
+ }
+
+ @keyframes pulse {
+ 0% {
+ transform: scale(0.5);
+ opacity: 0.8;
+ }
+ 100% {
+ transform: scale(3);
+ opacity: 0;
+ }
+ }
+`;
+
+const progressContext = createContext<[number, string]>([0, '']);
+
+const ProgressStatus = () => {
+ const [percent, progressText] = useContext(progressContext);
+
+ return (
+ <>
+ {percent > 0 ? (
+ <span>
+ ({percent}%{progressText && `, ${progressText}`})
+ </span>
+ ) : (
+ <>{progressText && <span>({progressText})</span>}</>
+ )}
+ </>
+ );
+};
+
+const ProgressSpin = () => {
+ const [percent] = useContext(progressContext);
+ return (
+ <>
+ {typeof percent === 'number' && percent > 0 && (
+ <StyledSpin size="small" percent={percent} />
+ )}
+ </>
+ );
+};
+
+const customDot: StepsProps['progressDot'] = (dot, { status }) => {
+ return status === 'process' ? (
+ <ActiveDot>
+ <ProgressSpin />
+ </ActiveDot>
+ ) : (
+ <>{dot}</>
+ );
+};
+
+const QueryStatusBar: FC<QueryStatusBarProps> = ({ query }) => {
+ const steps = [
+ {
+ title: t('Validate query'),
+ },
+ {
+ title: t('Connect to engine'),
+ },
+ {
+ title: (
+ <Flex align="center" gap="small">
+ {t('Running')}
+ <ProgressStatus />
+ </Flex>
+ ),
+ },
+ {
+ title: t('Download to client'),
+ },
+ {
+ title: t('Finish'),
+ },
+ ];
+
+ const hasError = useMemo(
+ () => ERROR_STATE.includes(query.state),
+ [query.state],
+ );
+ const prevStepRef = useRef<number>(0);
+ const progress =
+ query.progress > 0 ? parseInt(query.progress.toFixed(0), 10) : undefined;
Review Comment:
**Suggestion:** Calls `toFixed` on `query.progress` without ensuring it's a
number; if `query.progress` is undefined this will throw. Guard the access and
use a safe rounding method when `query.progress` is a number. [type error]
**Severity Level:** Minor ⚠️
```suggestion
typeof query.progress === 'number' && query.progress > 0
? Math.round(query.progress)
: undefined;
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
If query.progress can be undefined (or a non-number), calling toFixed could
throw or generate TypeScript errors. Guarding with a typeof check and using
Math.round is clearer and safer for both runtime and type-checking. The current
conditional (query.progress > 0) prevents the true-branch from executing when
progress is undefined at runtime, but an explicit typeof check is more robust
and explicit for maintainers and the type system.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/SqlLab/components/QueryStatusBar/index.tsx
**Line:** 157:157
**Comment:**
*Type Error: Calls `toFixed` on `query.progress` without ensuring it's
a number; if `query.progress` is undefined this will throw. Guard the access
and use a safe rounding method when `query.progress` is a number.
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/SqlLab/components/QueryStatusBar/index.tsx:
##########
@@ -0,0 +1,216 @@
+/**
+ * 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, createContext, useContext, useRef } from 'react';
+import { styled } from '@apache-superset/core';
+import {
+ Flex,
+ Steps,
+ type StepsProps,
+ StyledSpin,
+ Timer,
+} from '@superset-ui/core/components';
+import { QueryResponse, QueryState, t, usePrevious } from '@superset-ui/core';
+import QueryStateLabel from '../QueryStateLabel';
+
+type QueryStatusBarProps = {
+ query: QueryResponse;
+};
+
+const STATE_TO_STEP: Record<string, number> = {
+ offline: 4,
+ failed: 4,
+ pending: 0,
+ fetching: 3,
+ running: 2,
+ stopped: 4,
+ success: 4,
+};
+
+const ERROR_STATE = [QueryState.Failed, QueryState.Stopped];
+
+const StyledSteps = styled.div`
+ & .ant-steps {
+ margin: ${({ theme }) => theme.sizeUnit * 2}px 0;
+ }
+`;
+
+const ActiveDot = styled.div`
+ position: relative;
+ display: flex;
+ align-items: center;
+ justify-content: center;
+
+ &::before,
+ &::after {
+ content: '';
+ position: absolute;
+ width: 8px;
+ height: 8px;
+ border-radius: 50%;
+ background-color: ${({ theme }) => theme.colorPrimary};
+ top: -1px;
+ opacity: 0;
+ animation: pulse 2s ease-out infinite;
+ }
+
+ &::after {
+ animation-delay: 1s;
+ }
+
+ @keyframes pulse {
+ 0% {
+ transform: scale(0.5);
+ opacity: 0.8;
+ }
+ 100% {
+ transform: scale(3);
+ opacity: 0;
+ }
+ }
+`;
+
+const progressContext = createContext<[number, string]>([0, '']);
+
+const ProgressStatus = () => {
+ const [percent, progressText] = useContext(progressContext);
+
+ return (
+ <>
+ {percent > 0 ? (
+ <span>
+ ({percent}%{progressText && `, ${progressText}`})
+ </span>
+ ) : (
+ <>{progressText && <span>({progressText})</span>}</>
+ )}
+ </>
+ );
+};
+
+const ProgressSpin = () => {
+ const [percent] = useContext(progressContext);
+ return (
+ <>
+ {typeof percent === 'number' && percent > 0 && (
+ <StyledSpin size="small" percent={percent} />
+ )}
+ </>
+ );
+};
+
+const customDot: StepsProps['progressDot'] = (dot, { status }) => {
+ return status === 'process' ? (
+ <ActiveDot>
+ <ProgressSpin />
+ </ActiveDot>
+ ) : (
+ <>{dot}</>
+ );
+};
+
+const QueryStatusBar: FC<QueryStatusBarProps> = ({ query }) => {
+ const steps = [
+ {
+ title: t('Validate query'),
+ },
+ {
+ title: t('Connect to engine'),
+ },
+ {
+ title: (
+ <Flex align="center" gap="small">
+ {t('Running')}
+ <ProgressStatus />
+ </Flex>
+ ),
+ },
+ {
+ title: t('Download to client'),
+ },
+ {
+ title: t('Finish'),
+ },
+ ];
+
+ const hasError = useMemo(
+ () => ERROR_STATE.includes(query.state),
+ [query.state],
+ );
+ const prevStepRef = useRef<number>(0);
+ const progress =
+ query.progress > 0 ? parseInt(query.progress.toFixed(0), 10) : undefined;
+ const { progress_text: progressText } = query.extra;
Review Comment:
**Suggestion:** Destructuring `query.extra` without checking it can throw if
`extra` is undefined; access `progress_text` via optional chaining and provide
a safe default instead of directly destructuring from `query.extra`. [null
pointer]
**Severity Level:** Minor ⚠️
```suggestion
const progressText = query.extra?.progress_text ?? '';
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The current code destructures query.extra directly which will throw if
query.extra is ever undefined at runtime.
Switching to optional chaining is a small, safe defensive change that
prevents a potential crash and matches how the value is later used with a
fallback (progressText ?? ''). This also avoids TypeScript complaints if extra
is typed as optional.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/SqlLab/components/QueryStatusBar/index.tsx
**Line:** 158:158
**Comment:**
*Null Pointer: Destructuring `query.extra` without checking it can
throw if `extra` is undefined; access `progress_text` via optional chaining and
provide a safe default instead of directly destructuring from `query.extra`.
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]
