codeant-ai-for-open-source[bot] commented on code in PR #39257:
URL: https://github.com/apache/superset/pull/39257#discussion_r3662081618
##########
superset-frontend/src/components/Datasource/DatasourceModal/index.tsx:
##########
@@ -325,6 +332,8 @@ const DatasourceModal:
FunctionComponent<DatasourceModalProps> = ({
<StyledDatasourceModal
show={show}
onHide={onHide}
+ width={`${MODAL_WIDTH_VW}vw`}
+ maxWidth={`${MODAL_MAX_WIDTH}px`}
Review Comment:
**Suggestion:** The modal now gives Ant Design a fixed `90vw` width while
also placing it inside the resizable wrapper. Resizing changes the wrapper
dimensions, but the nested Ant modal continues to enforce `90vw`, so shrinking
the modal can leave the content overflowing the resized frame and expanding it
does not correctly resize the inner modal. The modal width must be driven by
the resizable dimensions or the fixed width must be removed when resizing is
enabled. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Dataset editor content can overflow after resizing.
- ⚠️ Resizing does not accurately control modal width.
- ⚠️ Footer and body alignment can become inconsistent.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the dataset editor through `DatasourceModal` at
`superset-frontend/src/components/Datasource/DatasourceModal/index.tsx:331`,
which renders
`StyledDatasourceModal` with `width="90vw"` and `maxWidth="1600px"` at lines
335-336.
2. The same modal enables `resizable` and supplies `defaultSize` through
`resizableConfig`
at
`superset-frontend/src/components/Datasource/DatasourceModal/index.tsx:385-394`.
3. `Modal` wraps the Ant Design modal inside `Resizable` at
`superset-frontend/packages/superset-ui-core/src/components/Modal/Modal.tsx:249-265`;
resizing changes the wrapper dimensions, while `modalWidth` remains the
fixed `width` prop
at lines 188 and 227-233.
4. Drag the resize handle to make the dataset modal narrower or wider and
observe that the
nested Ant modal continues using 90vw, causing overflow when narrowed and
preventing the
inner modal from matching the resized frame.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=82195057f16f4c209ec61cc250c5d9b6&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=82195057f16f4c209ec61cc250c5d9b6&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/components/Datasource/DatasourceModal/index.tsx
**Line:** 335:336
**Comment:**
*Api Mismatch: The modal now gives Ant Design a fixed `90vw` width
while also placing it inside the resizable wrapper. Resizing changes the
wrapper dimensions, but the nested Ant modal continues to enforce `90vw`, so
shrinking the modal can leave the content overflowing the resized frame and
expanding it does not correctly resize the inner modal. The modal width must be
driven by the resizable dimensions or the fixed width must be removed when
resizing is enabled.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39257&comment_hash=a52b70c9a675a7b10a7772fa6c8ad179724a12d618361d9210729282a01efbd3&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39257&comment_hash=a52b70c9a675a7b10a7772fa6c8ad179724a12d618361d9210729282a01efbd3&reaction=dislike'>👎</a>
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx:
##########
@@ -713,6 +744,18 @@ function ColumnCollectionTable({
),
type: d => (d ? <Label>{String(d)}</Label> : null),
advanced_data_type: d => <Label>{d as string}</Label>,
+ expression: (v, onChange) => (
+ <TextAreaControl
+ initialValue={v as string}
+ onChange={onChange}
Review Comment:
**Suggestion:** The inline SQL editor uses `initialValue`, which
`TextAreaControl` passes to Ace as `defaultValue`. Because collection rows
retain the same `id` when the parent collection is refreshed, the editor stays
mounted and does not update when the expression changes externally, leaving
stale SQL visible while the saved collection contains the refreshed value. Use
a controlled value or remount the editor when the expression changes. [stale
reference]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Calculated-column SQL can display stale values.
- ⚠️ Users may save an expression different from refreshed metadata.
- ⚠️ Dataset edits become difficult to reconcile visually.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the calculated-columns tab, rendered by `DatasourceEditor` at
`superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx:2487-2519`,
and edit an expression using the renderer at lines 787-798.
2. `ColumnCollectionTable` passes the current expression as `initialValue`
at lines
789-790 and sends edits through `onChange` at lines 790-791.
3. `TextAreaControl` converts `initialValue` to Ace's `defaultValue` at
`superset-frontend/src/explore/components/controls/TextAreaControl.tsx:112-126`,
so later
prop changes do not update the mounted editor value.
4. Trigger a collection refresh such as the metadata synchronization path at
`DatasourceEditor.tsx:1249-1297`, or receive a parent collection update
while the row
retains its id; `CollectionTable` updates its local row state at
`superset-frontend/src/components/Datasource/components/CollectionTable/index.tsx:62-68`,
but the mounted editor retains its previous Ace default value and can
display SQL
different from the refreshed datasource state.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=49772eaf78b541b1a7d7f1af578ab361&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=49772eaf78b541b1a7d7f1af578ab361&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx
**Line:** 749:750
**Comment:**
*Stale Reference: The inline SQL editor uses `initialValue`, which
`TextAreaControl` passes to Ace as `defaultValue`. Because collection rows
retain the same `id` when the parent collection is refreshed, the editor stays
mounted and does not update when the expression changes externally, leaving
stale SQL visible while the saved collection contains the refreshed value. Use
a controlled value or remount the editor when the expression changes.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39257&comment_hash=8bd59035fb7e2b13a0a7c85e7d73ed275ff175c72c0c68d12478a79d07b8a3f1&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39257&comment_hash=8bd59035fb7e2b13a0a7c85e7d73ed275ff175c72c0c68d12478a79d07b8a3f1&reaction=dislike'>👎</a>
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx:
##########
@@ -505,32 +533,29 @@ function ColumnCollectionTable({
filterTerm,
filterFields,
}: ColumnCollectionTableProps): JSX.Element {
+ const tableColumns = isFeatureEnabled(FeatureFlag.EnableAdvancedDataTypes)
+ ? [
+ 'column_name',
+ ...(showExpression ? ['expression'] : []),
+ 'advanced_data_type',
+ 'type',
+ 'is_dttm',
+ 'filterable',
+ 'groupby',
+ ]
+ : [
+ 'column_name',
+ ...(showExpression ? ['expression'] : []),
+ 'type',
+ 'is_dttm',
+ 'filterable',
+ 'groupby',
+ ];
+
return (
<CollectionTable
- tableColumns={
- isFeatureEnabled(FeatureFlag.EnableAdvancedDataTypes)
- ? [
- 'column_name',
- 'advanced_data_type',
- 'type',
- 'is_dttm',
- 'filterable',
- 'groupby',
- ]
- : ['column_name', 'type', 'is_dttm', 'filterable', 'groupby']
- }
- sortColumns={
- isFeatureEnabled(FeatureFlag.EnableAdvancedDataTypes)
- ? [
- 'column_name',
- 'advanced_data_type',
- 'type',
- 'is_dttm',
- 'filterable',
- 'groupby',
- ]
- : ['column_name', 'type', 'is_dttm', 'filterable', 'groupby']
- }
+ tableColumns={tableColumns}
+ sortColumns={tableColumns}
Review Comment:
**Suggestion:** Passing `tableColumns` as `sortColumns` enables sorting for
these tables, but `CollectionTable` resets an unsorted table from its original
`propsCollection` rather than its locally edited collection. Clearing a sort
after an inline edit can therefore discard the latest edit if the parent
datasource state has not propagated yet. Sorting must use the current edited
collection when resetting, or the local collection must be synchronized before
this reset. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Recent column edits can disappear when clearing sort.
- ⚠️ Calculated-column edits may be lost before saving.
- ⚠️ User-visible table state can diverge from datasource state.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the Columns or Calculated Columns tab, where `ColumnCollectionTable`
is rendered
at `DatasourceEditor.tsx:2421-2519`.
2. The new `sortColumns={tableColumns}` at `DatasourceEditor.tsx:557-558`
enables sorting
for every displayed column.
3. Edit a cell inline; `CollectionTable.onCellChange()` updates local state
and invokes
the parent callback at
`superset-frontend/src/components/Datasource/components/CollectionTable/index.tsx:70-102`,
while the parent datasource update is still being scheduled.
4. Immediately clear the active table sort.
`CollectionTable.handleTableChange()` resets
the array from `propsCollection` rather than the locally edited
`collectionArray` at
`CollectionTable/index.tsx:251-289`; if the parent prop has not propagated,
the edited
value disappears from the table and subsequent save state can omit that edit.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=6038c52a8b5148c88c319a44dbfedaf0&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=6038c52a8b5148c88c319a44dbfedaf0&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx
**Line:** 557:558
**Comment:**
*Logic Error: Passing `tableColumns` as `sortColumns` enables sorting
for these tables, but `CollectionTable` resets an unsorted table from its
original `propsCollection` rather than its locally edited collection. Clearing
a sort after an inline edit can therefore discard the latest edit if the parent
datasource state has not propagated yet. Sorting must use the current edited
collection when resetting, or the local collection must be synchronized before
this reset.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39257&comment_hash=83a99242230fe76a882af872d16628ad7f86fed0ba2954e077f4cd3834b01811&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39257&comment_hash=83a99242230fe76a882af872d16628ad7f86fed0ba2954e077f4cd3834b01811&reaction=dislike'>👎</a>
--
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]
