codeant-ai-for-open-source[bot] commented on code in PR #39257:
URL: https://github.com/apache/superset/pull/39257#discussion_r3664374952
##########
superset-frontend/src/components/Datasource/DatasourceModal/index.tsx:
##########
@@ -376,7 +385,11 @@ const DatasourceModal:
FunctionComponent<DatasourceModalProps> = ({
responsive
resizable
resizableConfig={{
- defaultSize: { width: 'auto', height: `${MODAL_HEIGHT_VH}vh` },
+ defaultSize: {
+ width: `${MODAL_WIDTH_VW}vw`,
+ height: `${MODAL_HEIGHT_VH}vh`,
+ },
+ maxWidth: `${MODAL_MAX_WIDTH}px`,
maxHeight: `${MODAL_HEIGHT_VH}vh`,
}}
Review Comment:
**Suggestion:** Providing a partial `resizableConfig` replaces the shared
modal's default configuration rather than merging with it, so the default
`minWidth` and `minHeight` constraints are removed. Users can resize the outer
wrapper below the size required by the header, footer, and the container's CSS
minimum, causing content and actions to overflow or become inaccessible.
Preserve explicit minimum constraints in this configuration. [css layout issue]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Manual resizing can clip dataset editor content.
- ❌ Footer Save and Cancel actions can become inaccessible.
- ⚠️ Header and SQL controls can overflow the wrapper.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the Edit Dataset modal rendered by
`superset-frontend/src/components/Datasource/DatasourceModal/index.tsx:331-404`;
the modal
enables resizing at `index.tsx:385-386`.
2. The PR supplies the partial configuration at `index.tsx:387-394`,
omitting `minWidth`
and `minHeight`.
3.
`superset-frontend/packages/superset-ui-core/src/components/Modal/Modal.tsx:334-339`
returns a supplied non-empty `resizableConfig` unchanged rather than merging
it with
`defaultResizableConfig`.
4. The resulting configuration is spread into `Resizable` at
`Modal.tsx:389-405`, so
dragging the resize handle can reduce the outer wrapper below the shared
defaults (`380px`
width and header/footer-aware minimum height defined at `Modal.tsx:217-230`).
5. The inner container still has a 500px desktop minimum at
`index.tsx:58-67`, allowing
the modal contents and footer actions to overflow the resized wrapper and
become clipped
or inaccessible.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e24ef1f94e924e7d86346e95ecc6afdb&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=e24ef1f94e924e7d86346e95ecc6afdb&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:** 387:394
**Comment:**
*Css Layout Issue: Providing a partial `resizableConfig` replaces the
shared modal's default configuration rather than merging with it, so the
default `minWidth` and `minHeight` constraints are removed. Users can resize
the outer wrapper below the size required by the header, footer, and the
container's CSS minimum, causing content and actions to overflow or become
inaccessible. Preserve explicit minimum constraints in this configuration.
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=71c3f6cb345e12ddea9432a21ae291bf0dc4ffe68829a437af40c5039e9c940c&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39257&comment_hash=71c3f6cb345e12ddea9432a21ae291bf0dc4ffe68829a437af40c5039e9c940c&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}
+ className="datasource-sql-expression"
+ language="sql"
+ offerEditInModal={false}
+ minLines={5}
+ textAreaStyles={{ minWidth: '100%', maxWidth: 'none' }}
+ resize="both"
+ />
Review Comment:
**Suggestion:** The inline SQL editor uses `initialValue`, which is passed
to `TextAreaEditor` as `defaultValue`. Because the editor component is
preserved when `CollectionTable` synchronizes a row with new datasource or
metadata props, its displayed SQL does not update even though the collection
and save payload do. Use a controlled/update-aware value or remount the editor
when the row value changes. [stale reference]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Calculated-column SQL can become visually stale.
- ❌ Editing stale SQL can overwrite refreshed expressions.
- ⚠️ Metadata synchronization can leave editor and payload inconsistent.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the Calculated Columns tab, which renders `ColumnCollectionTable`
from
`superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx:2487-2519`.
2. Each expression cell creates `TextAreaControl` with `initialValue={v}` at
`DatasourceEditor.tsx:747-757`; `TextAreaControl` passes that value to
`TextAreaEditor` as
`defaultValue` at
`superset-frontend/src/explore/components/controls/TextAreaControl.tsx:171-184`.
3. When the datasource or metadata refreshes, `CollectionTable` synchronizes
its internal
rows from `propsCollection` in
`superset-frontend/src/components/Datasource/components/CollectionTable/index.tsx:131-137`,
but it preserves the existing row/editor identity.
4. `TextAreaControl` does not remount based on the expression value: its
editor key is
only `name` at `TextAreaControl.tsx:180-183`, and these renderers do not
provide a
row-specific `name` at `DatasourceEditor.tsx:747-757`.
5. The collection and eventual save payload can therefore contain the
refreshed expression
while the uncontrolled Ace editor continues displaying its previous
`defaultValue`,
producing stale SQL in the visible editor and potentially overwriting
refreshed data on a
subsequent edit.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=cd5602ddeda34a4e9064a69f820097ec&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=cd5602ddeda34a4e9064a69f820097ec&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:** 747:757
**Comment:**
*Stale Reference: The inline SQL editor uses `initialValue`, which is
passed to `TextAreaEditor` as `defaultValue`. Because the editor component is
preserved when `CollectionTable` synchronizes a row with new datasource or
metadata props, its displayed SQL does not update even though the collection
and save payload do. Use a controlled/update-aware value or remount the editor
when the row value 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=8943a59b10ab62a60f60ec836ae9f16ad60523de355ffb5b8dd71680b3aca26b&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39257&comment_hash=8943a59b10ab62a60f60ec836ae9f16ad60523de355ffb5b8dd71680b3aca26b&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]
