codeant-ai-for-open-source[bot] commented on code in PR #37396:
URL: https://github.com/apache/superset/pull/37396#discussion_r2758392571
##########
superset-frontend/src/explore/components/DataTablesPane/types.ts:
##########
@@ -78,6 +76,7 @@ export interface TableControlsProps {
export interface QueryResultInterface {
colnames: string[];
+ collabels: string[];
Review Comment:
**Suggestion:** The `collabels` field is declared as a required `string[]`,
but column labels are an optional enhancement ("show a label if provided,
falling back to the column name"), so typing this as always present can
misrepresent the actual API contract and force callers to fabricate arrays
instead of recognizing the absence of labels; making it optional better matches
the intended semantics and avoids downstream logic relying on a non-existent
value. [type error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Frontend TypeScript compilation fails on query result consumers.
- ❌ Components using QueryResultInterface require fabricated labels.
- ⚠️ SamplesPane and SingleQueryResultPane type contracts affected.
- ⚠️ CI type-check step will catch these errors.
```
</details>
```suggestion
collabels?: string[];
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start a TypeScript build / dev server for the frontend (e.g., run `yarn
start` or `yarn
build` in the repo root) which triggers type-checking for the frontend
codebase.
2. Open file
`superset-frontend/src/explore/components/DataTablesPane/types.ts` and
inspect the QueryResultInterface declaration at the hunk shown in the PR:
`export
interface QueryResultInterface {` (line 77 in the PR hunk) and the added
property
`collabels: string[];` at line 79. This property is declared as required.
3. In the same file `types.ts`, `export interface SingleQueryResultPaneProp
extends
QueryResultInterface {` (immediately following the QueryResultInterface
block, line ~84 in
the hunk context) inherits that required `collabels` property. Any value
typed as
`SingleQueryResultPaneProp` must therefore include `collabels`.
4. When the project is compiled, TypeScript will report errors in any module
that
constructs or receives query-result-shaped objects (for example, API result
handling code,
`useResultsPane`, or components consuming `SamplesPaneProps`) but does not
provide
`collabels`. The compiler error appears similar to: "Property 'collabels' is
missing in
type 'X' but required in type 'QueryResultInterface'". This breaks local
type-checking and
CI type checks until all call sites provide `collabels`.
Note: The PR description indicates `collabels` is intended as an optional
enhancement
("show a label if provided, falling back to the column name"). Making the
field required
therefore conflicts with the stated design and forces callers to fabricate
empty arrays
instead of handling absence. The existing code pattern in the repo (features
that fall
back to colname when no label exists) suggests the property should be
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/explore/components/DataTablesPane/types.ts
**Line:** 79:79
**Comment:**
*Type Error: The `collabels` field is declared as a required
`string[]`, but column labels are an optional enhancement ("show a label if
provided, falling back to the column name"), so typing this as always present
can misrepresent the actual API contract and force callers to fabricate arrays
instead of recognizing the absence of labels; making it optional better matches
the intended semantics and avoids downstream logic relying on a non-existent
value.
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/explore/components/DataTablesPane/components/useResultsPane.tsx:
##########
@@ -159,13 +158,13 @@ export const useResultsPane = ({
<SingleQueryResultPane
data={result.data}
colnames={result.colnames}
+ collabels={result.collabels}
Review Comment:
**Suggestion:** The `collabels` field coming from the query result is likely
optional for some responses, so passing `result.collabels` through unchanged
can result in `undefined` being sent to child components that may expect an
array and call array methods on it, causing runtime errors; normalizing it to a
safe default avoids this. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Explore DataTablesPane table headers crash (useResultsPane).
- ⚠️ SingleQueryResultPane receives undefined collabels.
- ⚠️ Header-label rendering tests may fail intermittently.
```
</details>
```suggestion
collabels={result.collabels ?? []}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Build and run the frontend containing this PR.
2. In the Explore UI run a query that returns a result object where the
per-result entry
lacks a `collabels` property (this response is consumed by
getChartDataRequest imported
from `src/components/Chart/chartAction` and then handled in
`superset-frontend/src/explore/components/DataTablesPane/components/useResultsPane.tsx`
—
see the `.then(({ json }) => { setResultResp(ensureIsArray(json.result));
... })` handler
in the same file).
3. After the response, the component maps `resultRespToDisplay` and renders
`SingleQueryResultPane` — the instantiation is in `useResultsPane.tsx`
around the mapping
block, specifically the prop is passed at line 161:
`collabels={result.collabels}`.
4. If `SingleQueryResultPane` implementation assumes `collabels` is an array
(for example
it calls `collabels.map(...)`), it will receive `undefined` and throw a
TypeError at
render time. Normalizing the prop (e.g. `result.collabels ?? []`) prevents
this runtime
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/explore/components/DataTablesPane/components/useResultsPane.tsx
**Line:** 161:161
**Comment:**
*Possible Bug: The `collabels` field coming from the query result is
likely optional for some responses, so passing `result.collabels` through
unchanged can result in `undefined` being sent to child components that may
expect an array and call array methods on it, causing runtime errors;
normalizing it to a safe default avoids this.
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/explore/components/DataTableControl/index.tsx:
##########
@@ -305,13 +330,13 @@ export const useTableColumns = (
.map((key, index) => {
const colType = coltypes?.[index];
const firstValue = data[0][key];
- const headerLabel = columnDisplayNames?.[key] ?? key;
const originalFormattedTimeColumnIndex =
colType === GenericDataType.Temporal
? originalFormattedTimeColumns.indexOf(key)
: -1;
const isOriginalTimeColumn =
originalFormattedTimeColumns.includes(key);
+ const label = collabels?.[index];
Review Comment:
**Suggestion:** The label array is indexed using the filtered column index,
so when some column names are filtered out (because they are not present in the
data), the labels become misaligned with the visible columns; the fix is to
compute the index in the original `colnames` array before filtering and use
that to look up the corresponding label. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ DataTableControl header labels display incorrect text.
- ⚠️ Explore SingleQueryResultPane shows misaligned labels.
- ⚠️ SamplesPane table headers may display wrong labels.
```
</details>
```suggestion
const originalIndex = (colnames || []).indexOf(key);
const label =
originalIndex > -1 ? collabels?.[originalIndex] : undefined;
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Add a small test component that calls the exported hook `useTableColumns`
from
`superset-frontend/src/explore/components/DataTableControl/index.tsx` (the
hook
declaration begins at the hunk starting around line 279 in this file).
Render the hook
inside a React test render so it executes the mapping code at lines ~330-356.
2. Pass these props into `useTableColumns` (this calls the mapping at lines
~330-356,
where label is read at line 339):
- colnames = ['colA', 'colB', 'colC']
- collabels = ['Label A', 'Label B', 'Label C']
- data = [{ colA: 1, colC: 3 }] // first row missing 'colB'
3. Inspect the returned columns' Header render output. The mapping executed
at `index.tsx`
around lines 330-356 uses the local `index` from the filtered `.map((key,
index) => ...)`
and executes `const label = collabels?.[index];` (line 339). Because `colB`
is filtered
out (missing from data[0]), the filtered map indices shift and the label for
'colC' will
be taken from `collabels[1]` ('Label B') instead of `collabels[2]` ('Label
C').
4. Observe the misaligned header label in the rendered DOM: header for
column 'colC' shows
'Label B' (incorrect). This reproduces the bug deterministically in a
unit/integration
test that renders `useTableColumns` (no hypothetical inputs required).
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/explore/components/DataTableControl/index.tsx
**Line:** 339:339
**Comment:**
*Logic Error: The label array is indexed using the filtered column
index, so when some column names are filtered out (because they are not present
in the data), the labels become misaligned with the visible columns; the fix is
to compute the index in the original `colnames` array before filtering and use
that to look up the corresponding label.
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]
