codeant-ai-for-open-source[bot] commented on code in PR #37408:
URL: https://github.com/apache/superset/pull/37408#discussion_r3485750191
##########
superset-frontend/src/SqlLab/components/ResultSet/index.tsx:
##########
@@ -758,22 +754,18 @@ const ResultSet = ({
/>
)}
</div>
- {useFixedHeight && height !== undefined ? (
- <ResultTable {...tableProps} height={height} />
- ) : (
- <div
- css={css`
- flex: 1 1 auto;
- padding-bottom: ${theme.sizeUnit * 3}px;
- `}
- >
- <AutoSizer disableWidth>
- {({ height: autoHeight }) => (
- <ResultTable {...tableProps} height={autoHeight} />
- )}
- </AutoSizer>
- </div>
- )}
+ <div
+ css={css`
+ flex: 1 1 auto;
+ padding-bottom: ${theme.sizeUnit * 3}px;
+ `}
+ >
Review Comment:
**Suggestion:** The new AutoSizer host is a flex child but does not set
`min-height: 0`; in nested flex layouts this prevents shrinking and causes
incorrect measured height/overflow behavior, which can clip the table bottom.
Add `min-height: 0` on this flex container so AutoSizer gets the actual
available space. [css layout issue]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
⚠️ ResultSet table height mismeasured, clipping bottom rows.
⚠️ Scroll behaviour inconsistent in history modal versus main results.
⚠️ AutoSizer sizing unreliable in nested flex layout.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In the Query History modal path described in suggestion 2, `ResultSet` is
rendered
inside `ModalResultSetWrapper` (flex column, `height: 500px`) at
`superset-frontend/src/SqlLab/components/QueryTable/styles.ts:43-47`, giving
the inner
`ResultSet` a fixed vertical space.
2. Within `ResultSet`, `ResultContainer` is a flex column container with
`height: 100%` at
`superset-frontend/src/SqlLab/components/ResultSet/index.tsx:118-123`. When
query results
are present, the success branch at
`superset-frontend/src/SqlLab/components/ResultSet/index.tsx:679-779`
renders two children
inside `ResultContainer`: (a) the top controls row at `703-756` and (b) a
second `div`
that hosts the table, styled with `flex: 1 1 auto; padding-bottom:
${theme.sizeUnit *
3}px;` but no `min-height: 0` at
`superset-frontend/src/SqlLab/components/ResultSet/index.tsx:757-762`.
3. In nested flex layouts, a child with `flex: 1 1 auto` and the default
`min-height:
auto` cannot shrink below its content height; when the inner `ResultTable`
(AG Grid /
FilterableTable) creates its own scrollable area inside this flex child, the
child’s
intrinsic height can exceed the available 500px from
`ModalResultSetWrapper`. AutoSizer at
`superset-frontend/src/SqlLab/components/ResultSet/index.tsx:763-767`
measures this
inflated scroll height rather than the actual visible space.
4. On tall result sets (many rows), open the Query History modal;
AutoSizer’s `height`
measurement based on the non-shrinking flex child causes the table to render
with a height
larger than the visible space inside `ModalResultSetWrapper`, leading to the
bottom rows
and horizontal scroll area being cut off. Adding `min-height: 0` to this
flex child at
`superset-frontend/src/SqlLab/components/ResultSet/index.tsx:757-762` allows
it to shrink
to the true available height so AutoSizer reports the correct `autoHeight`
and the table
content fits without clipping.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=2117ad070f9345a08660df0fe4f4e5f5&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=2117ad070f9345a08660df0fe4f4e5f5&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/SqlLab/components/ResultSet/index.tsx
**Line:** 759:762
**Comment:**
*Css Layout Issue: The new AutoSizer host is a flex child but does not
set `min-height: 0`; in nested flex layouts this prevents shrinking and causes
incorrect measured height/overflow behavior, which can clip the table bottom.
Add `min-height: 0` on this flex container so AutoSizer gets the actual
available space.
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%2F37408&comment_hash=e965a00c710ca6e1d176c9d1cc0718b397ecfe1d93e65a917a12e63e2aa1b7f7&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37408&comment_hash=e965a00c710ca6e1d176c9d1cc0718b397ecfe1d93e65a917a12e63e2aa1b7f7&reaction=dislike'>👎</a>
##########
superset-frontend/src/SqlLab/components/QueryTable/index.tsx:
##########
@@ -407,24 +407,12 @@ const QueryTable = ({
modalBody={
selectedQuery ? (
<ModalResultSetWrapper>
- {(() => {
- const height =
- reduxQueries[selectedQuery.id]?.state ===
- QueryState.Success &&
- reduxQueries[selectedQuery.id]?.results
- ? Math.floor(window.innerHeight * 0.5)
- : undefined;
- return (
- <ResultSet
- showSql
- queryId={selectedQuery.id}
- displayLimit={displayLimit}
- defaultQueryLimit={1000}
- useFixedHeight
- height={height}
- />
- );
- })()}
+ <ResultSet
+ showSql
+ queryId={selectedQuery.id}
+ displayLimit={displayLimit}
+ defaultQueryLimit={1000}
+ />
Review Comment:
**Suggestion:** `defaultQueryLimit` is hardcoded to `1000` when opening
query-history results, but the actual SQL Lab default limit is configurable
(`common.conf.DEFAULT_SQLLAB_LIMIT`). When deployments use a different default,
ResultSet warning logic will classify limit reasons incorrectly and show wrong
limit messaging. Pass the real configured default limit into `QueryTable` and
forward that value instead of hardcoding. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
⚠️ Query history modal shows misleading row limit reason.
⚠️ Results pane and history tab give inconsistent limit messages.
⚠️ Deployments customizing DEFAULT_SQLLAB_LIMIT see confusing UX.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure SQL Lab to use a non-1000 default limit by setting
`common.conf.DEFAULT_SQLLAB_LIMIT` (e.g. 5000) in Redux initial state, as
consumed in
`mapStateToProps` of `TabbedSqlEditors` at
`superset-frontend/src/SqlLab/components/TabbedSqlEditors/index.tsx:100-115`
where
`defaultQueryLimit` is mapped from `common.conf.DEFAULT_SQLLAB_LIMIT`.
2. Open SQL Lab, run a query using the default row limit from the limit
dropdown (no
`LIMIT` clause override), and observe the main Results pane: `SouthPane`
renders `Results`
at `superset-frontend/src/SqlLab/components/SouthPane/index.tsx:169-176`,
and `Results`
passes `defaultQueryLimit` down to `ResultSet` at
`superset-frontend/src/SqlLab/components/SouthPane/Results.tsx:96-107`.
3. In `ResultSet`
(`superset-frontend/src/SqlLab/components/ResultSet/index.tsx:490-527`),
`renderRowsReturned()` uses `defaultQueryLimit` together with
`limitingFactor` and
`queryLimit` to compute `shouldUseDefaultDropdownAlert` and build a specific
`limitMessage` for the main Results pane based on the configured default.
4. Navigate to the Query History tab; `QueryHistory` at
`superset-frontend/src/SqlLab/components/QueryHistory/index.tsx:125-20`
renders
`QueryTable` with `displayLimit` but not `defaultQueryLimit`. Click a row’s
“View” button
(created in `QueryTable` at
`superset-frontend/src/SqlLab/components/QueryTable/index.tsx:315-328`),
which opens the
modal rendering `ResultSet` via `ModalResultSetWrapper`. In this modal
`ResultSet` is
instantiated with `defaultQueryLimit={1000}` at
`superset-frontend/src/SqlLab/components/QueryTable/index.tsx:410-415`, so
`renderRowsReturned()` now evaluates `shouldUseDefaultDropdownAlert` against
1000 instead
of the real configured default (e.g. 5000), leading to inconsistent and
misleading
limit-reason messaging between the main Results pane and the Query History
modal for the
same query.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4cee861ad9cf46d9bc9904c07ab2ae9d&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=4cee861ad9cf46d9bc9904c07ab2ae9d&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/SqlLab/components/QueryTable/index.tsx
**Line:** 410:415
**Comment:**
*Logic Error: `defaultQueryLimit` is hardcoded to `1000` when opening
query-history results, but the actual SQL Lab default limit is configurable
(`common.conf.DEFAULT_SQLLAB_LIMIT`). When deployments use a different default,
ResultSet warning logic will classify limit reasons incorrectly and show wrong
limit messaging. Pass the real configured default limit into `QueryTable` and
forward that value instead of hardcoding.
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%2F37408&comment_hash=0fe6173ae4aed29a8b4ae84d5963861177ef4187e3c89ecfc2dab4ed7100b69c&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37408&comment_hash=0fe6173ae4aed29a8b4ae84d5963861177ef4187e3c89ecfc2dab4ed7100b69c&reaction=dislike'>👎</a>
##########
superset-frontend/src/SqlLab/components/QueryTable/styles.ts:
##########
@@ -44,4 +44,5 @@ export const ModalResultSetWrapper = styled.div`
display: flex;
flex-direction: column;
overflow: hidden;
+ height: 500px;
Review Comment:
**Suggestion:** Setting a fixed `500px` modal body height makes the result
view non-responsive and can clip content on shorter viewports, especially since
the wrapper also hides overflow. Use a viewport-relative/max-height strategy so
the modal remains usable across screen sizes. [css layout issue]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
⚠️ Data preview modal unusable on short viewport screens.
⚠️ Bottom table rows clipped in query history modal.
⚠️ Streaming export controls may be hidden from some users.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Note that `SouthPane` uses `QueryHistory` to render Query History tab
content at
`superset-frontend/src/SqlLab/components/SouthPane/index.tsx:20-32`, and
`QueryHistory`
instantiates `QueryTable` with `displayLimit` and `latestQueryId` at
`superset-frontend/src/SqlLab/components/QueryHistory/index.tsx:120-20`.
2. In `QueryTable`
(`superset-frontend/src/SqlLab/components/QueryTable/index.tsx:383-41`),
query results are
opened in a modal via `ModalTrigger`, whose `modalBody` is
`ModalResultSetWrapper`
wrapping `ResultSet` at
`superset-frontend/src/SqlLab/components/QueryTable/index.tsx:28-38`.
3. `ModalResultSetWrapper` is styled as a flex column container with
`overflow: hidden`
and a fixed `height: 500px` at
`superset-frontend/src/SqlLab/components/QueryTable/styles.ts:43-47`. On
shorter viewports
where the modal’s available vertical space is <500px, this inner wrapper’s
fixed height
exceeds the visible area while `overflow: hidden` prevents scrolling inside
the wrapper.
4. On such a short viewport, open SQL Lab, go to Query History, and click
“View” on a
query with many rows. The `ResultSet` content (table and streaming export
modal) rendered
inside `ModalResultSetWrapper` is clipped to the available viewport height
and cannot
scroll because `overflow: hidden` is set on a 500px-tall container, making
the bottom of
the result table and some controls inaccessible to users on smaller screens.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=09c0480325fb44d2b0a46ef963f53272&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=09c0480325fb44d2b0a46ef963f53272&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/SqlLab/components/QueryTable/styles.ts
**Line:** 47:47
**Comment:**
*Css Layout Issue: Setting a fixed `500px` modal body height makes the
result view non-responsive and can clip content on shorter viewports,
especially since the wrapper also hides overflow. Use a
viewport-relative/max-height strategy so the modal remains usable across screen
sizes.
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%2F37408&comment_hash=1edbbe7b4583288ced92e5b2cd708c27e077562de22bd9088aea963503a6f512&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37408&comment_hash=1edbbe7b4583288ced92e5b2cd708c27e077562de22bd9088aea963503a6f512&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]
