You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "ramiroaquinoromero (via GitHub)" <gi...@apache.org> on 2026/01/14 02:31:26 UTC
[PR] fix: charts row limit warning is missing for server [sc-89835] [superset]
ramiroaquinoromero opened a new pull request, #37112:
URL: https://github.com/apache/superset/pull/37112
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: charts row limit warning is missing for server [sc-89835] [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37112:
URL: https://github.com/apache/superset/pull/37112#issuecomment-3762662650
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: charts row limit warning is missing for server [sc-89835] [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #37112:
URL: https://github.com/apache/superset/pull/37112#discussion_r2704018136
##########
superset-frontend/src/dashboard/components/SliceHeader/SliceHeader.test.tsx:
##########
@@ -677,8 +671,187 @@ test('Should show RowCountLabel in embedded when uiConfig.showRowLimitWarning is
initialState: rowCountState,
});
- expect(screen.getByTestId('warning')).toBeInTheDocument();
+ expect(screen.queryByTestId('warning')).not.toBeInTheDocument();
+ expect(screen.queryByTestId('row-count-label')).not.toBeInTheDocument();
mockIsEmbedded.mockRestore();
mockUseUiConfig.mockRestore();
});
+
+test('Should show row count badge for table chart without server pagination', () => {
+ const mockUseUiConfig = useUiConfig as jest.MockedFunction<
+ typeof useUiConfig
+ >;
+ mockUseUiConfig.mockReturnValue({
+ hideTitle: false,
+ hideTab: false,
+ hideNav: false,
+ hideChartControls: false,
+ emitDataMasks: false,
+ showRowLimitWarning: true,
+ });
+
+ const props = createProps({
+ formData: {
+ ...createProps().formData,
+ viz_type: VizType.Table,
+ row_limit: 10,
+ },
+ slice: {
+ ...createProps().slice,
+ form_data: {
+ ...createProps().slice.form_data,
+ viz_type: VizType.Table,
+ row_limit: 10,
+ },
+ viz_type: VizType.Table,
+ },
Review Comment:
Also why are we singling out the row limit for table charts?
##########
superset-frontend/src/dashboard/components/SliceHeader/index.tsx:
##########
@@ -304,18 +316,20 @@ const SliceHeader = forwardRef<HTMLDivElement, SliceHeaderProps>(
<FiltersBadge chartId={slice.slice_id} />
)}
- {shouldShowRowLimitWarning && sqlRowCount === rowLimit && (
+ {showRowCountLabel && (
<RowCountLabel
rowcount={sqlRowCount}
limit={rowLimit}
label={
- <Icons.WarningOutlined
- iconSize="l"
- iconColor={theme.colorWarning}
- css={theme => css`
- padding: ${theme.sizeUnit}px;
- `}
- />
+ showRowLimitWarning ? (
+ <Icons.WarningOutlined
+ iconSize="l"
+ iconColor={theme.colorWarning}
+ css={theme => css`
+ padding: ${theme.sizeUnit}px;
+ `}
+ />
+ ) : undefined
Review Comment:
I think this is one level higher than where it should be. RowCountLabel already has the row limit warning check implemented and this icon is displayed according to that. Can we check if we need to modify the logic inside `RowCountLabel` instead?
##########
superset-frontend/src/dashboard/components/SliceHeader/index.tsx:
##########
@@ -186,14 +186,24 @@ const SliceHeader = forwardRef<HTMLDivElement, SliceHeaderProps>(
({ dashboardInfo }) => dashboardInfo.crossFiltersEnabled,
);
- const firstQueryResponse = useSelector<RootState, QueryData | undefined>(
- state => state.charts[slice.slice_id].queriesResponse?.[0],
+ const queriesResponse = useSelector<RootState, QueryData[] | null | undefined>(
+ state => state.charts[slice.slice_id].queriesResponse,
);
const theme = useTheme();
- const rowLimit = Number(formData.row_limit || -1);
- const sqlRowCount = Number(firstQueryResponse?.sql_rowcount || 0);
+ const rowLimit = Number(formData.row_limit ?? 0);
Review Comment:
I implemented this in [this pr](https://github.com/apache/superset/pull/33781). Can we check if the functionality there haven't regressed? This -1 was for that.
##########
superset-frontend/src/dashboard/components/SliceHeader/SliceHeader.test.tsx:
##########
@@ -565,7 +565,7 @@ test('Add extension to SliceHeader', () => {
expect(screen.getByText('This is an extension')).toBeInTheDocument();
});
-test('Should render RowCountLabel when row limit is hit, and hide it otherwise', () => {
+test('Should NOT render warning for non-table chart when row limit is hit', () => {
Review Comment:
It seems a bit wrong to change this. I don't think the requirements have changed for it.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: charts row limit warning is missing for server [superset]
Posted by "ramiroaquinoromero (via GitHub)" <gi...@apache.org>.
ramiroaquinoromero commented on code in PR #37112:
URL: https://github.com/apache/superset/pull/37112#discussion_r2730310575
##########
superset-frontend/src/dashboard/components/SliceHeader/index.tsx:
##########
@@ -304,18 +316,20 @@ const SliceHeader = forwardRef<HTMLDivElement, SliceHeaderProps>(
<FiltersBadge chartId={slice.slice_id} />
)}
- {shouldShowRowLimitWarning && sqlRowCount === rowLimit && (
+ {showRowCountLabel && (
<RowCountLabel
rowcount={sqlRowCount}
limit={rowLimit}
label={
- <Icons.WarningOutlined
- iconSize="l"
- iconColor={theme.colorWarning}
- css={theme => css`
- padding: ${theme.sizeUnit}px;
- `}
- />
+ showRowLimitWarning ? (
+ <Icons.WarningOutlined
+ iconSize="l"
+ iconColor={theme.colorWarning}
+ css={theme => css`
+ padding: ${theme.sizeUnit}px;
+ `}
+ />
+ ) : undefined
Review Comment:
Good morning @msyavuz this is a good point. However, the logic needs to stay in SliceHeader because:
RowCountLabel is a generic component - it doesn't know about server pagination or how to extract the total count from the second query response (lines 198-206). This extraction logic is the actual fix for the bug.
The showRowLimitWarning check determines the visual presentation - it decides whether to show just the warning icon or the default row count label. RowCountLabel's internal limitReached logic then wraps whatever we pass with the tooltip.
Separation of concerns - Moving this logic into RowCountLabel would require passing queriesResponse, formData, and chart-specific context, making it tightly coupled to dashboard internals. It's currently also used in SqlLab where this logic wouldn't apply.
The key fix is passing the correct sqlRowCount (from the second query when server pagination is enabled) to RowCountLabel, which already handles the tooltip logic correctly.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: charts row limit warning is missing for server [sc-89835] [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #37112:
URL: https://github.com/apache/superset/pull/37112#issuecomment-3762605022
CodeAnt AI is reviewing your PR.
---
### Thanks for using CodeAnt! π
We're free for open-source projects. if you're enjoying it, help us grow by sharing.
[Share on X](https://twitter.com/intent/tweet?text=Just%20tried%20%40CodeAntAI%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A&url=https%3A//codeant.ai) Β·
[Reddit](https://www.reddit.com/submit?title=Check%20out%20CodeAnt%20for%20automated%20code%20review&text=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A%20https%3A//codeant.ai) Β·
[LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcodeant.ai&mini=true&title=Check%20out%20CodeAnt%20for%20automated%20code%20review&summary=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: charts row limit warning is missing for server [superset]
Posted by "dirivasm (via GitHub)" <gi...@apache.org>.
dirivasm commented on PR #37112:
URL: https://github.com/apache/superset/pull/37112#issuecomment-3837107697
# Test Summary
The tooltip is now displayed when the row limit is set and server pagination is enabled :white_check_mark:
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: charts row limit warning is missing for server [sc-89835] [superset]
Posted by "reynoldmorel (via GitHub)" <gi...@apache.org>.
reynoldmorel commented on code in PR #37112:
URL: https://github.com/apache/superset/pull/37112#discussion_r2700560283
##########
superset-frontend/src/dashboard/components/SliceHeader/index.tsx:
##########
@@ -186,14 +186,24 @@ const SliceHeader = forwardRef<HTMLDivElement, SliceHeaderProps>(
({ dashboardInfo }) => dashboardInfo.crossFiltersEnabled,
);
- const firstQueryResponse = useSelector<RootState, QueryData | undefined>(
- state => state.charts[slice.slice_id].queriesResponse?.[0],
+ const queriesResponse = useSelector<RootState, QueryData[] | null | undefined>(
+ state => state.charts[slice.slice_id].queriesResponse,
);
const theme = useTheme();
- const rowLimit = Number(formData.row_limit || -1);
- const sqlRowCount = Number(firstQueryResponse?.sql_rowcount || 0);
+ const rowLimit = Number(formData.row_limit ?? 0);
Review Comment:
Why are we changing this from `-1` to `0`, are you sure this is not breaking any existing logic?
I agree with this change though, if it is safe xD!
##########
superset-frontend/src/dashboard/components/SliceHeader/index.tsx:
##########
@@ -304,18 +314,20 @@ const SliceHeader = forwardRef<HTMLDivElement, SliceHeaderProps>(
<FiltersBadge chartId={slice.slice_id} />
)}
- {shouldShowRowLimitWarning && sqlRowCount === rowLimit && (
+ {!uiConfig.hideChartControls && isTableChart && sqlRowCount > 0 && (
<RowCountLabel
rowcount={sqlRowCount}
limit={rowLimit}
label={
- <Icons.WarningOutlined
- iconSize="l"
- iconColor={theme.colorWarning}
- css={theme => css`
- padding: ${theme.sizeUnit}px;
- `}
- />
+ shouldShowRowLimitWarning && sqlRowCount >= rowLimit && rowLimit > 0 ? (
Review Comment:
nit: Also make these flags variables to understand a bit faster what they are doing. For example:
```
const showWarningMsg = showRowCountLabel && sqlRowCount >= rowLimit && rowLimit > 0;
```
##########
superset-frontend/src/dashboard/components/SliceHeader/index.tsx:
##########
@@ -304,18 +314,20 @@ const SliceHeader = forwardRef<HTMLDivElement, SliceHeaderProps>(
<FiltersBadge chartId={slice.slice_id} />
)}
- {shouldShowRowLimitWarning && sqlRowCount === rowLimit && (
+ {!uiConfig.hideChartControls && isTableChart && sqlRowCount > 0 && (
Review Comment:
nit: Also make these flags variables to understand a bit faster what they are doing. For example:
```
const showRowCountLabel = !uiConfig.hideChartControls && isTableChart && sqlRowCount > 0;
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: charts row limit warning is missing for server [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37112:
URL: https://github.com/apache/superset/pull/37112#issuecomment-3803524849
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: charts row limit warning is missing for server [superset]
Posted by "ramiroaquinoromero (via GitHub)" <gi...@apache.org>.
ramiroaquinoromero commented on PR #37112:
URL: https://github.com/apache/superset/pull/37112#issuecomment-3780471469
> Superset uses Git pre-commit hooks courtesy of [pre-commit](https://pre-commit.com/). To install run the following:
>
> ```
> pip3 install -r requirements/development.txt
> pre-commit install
> ```
>
> A series of checks will now run when you make a git commit.
>
> Alternatively it is possible to run pre-commit by running pre-commit manually:
>
> ```
> pre-commit run --all-files
> ```
Yup I am validating this. Thank you.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: charts row limit warning is missing for server [sc-89835] [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #37112:
URL: https://github.com/apache/superset/pull/37112#issuecomment-3762607692
CodeAnt AI finished reviewing your PR.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: charts row limit warning is missing for server [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on PR #37112:
URL: https://github.com/apache/superset/pull/37112#issuecomment-3780363665
Superset uses Git pre-commit hooks courtesy of [pre-commit](https://pre-commit.com/). To install run the following:
```
pip3 install -r requirements/development.txt
pre-commit install
```
A series of checks will now run when you make a git commit.
Alternatively it is possible to run pre-commit by running pre-commit manually:
```
pre-commit run --all-files
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: charts row limit warning is missing for server [superset]
Posted by "ramiroaquinoromero (via GitHub)" <gi...@apache.org>.
ramiroaquinoromero commented on code in PR #37112:
URL: https://github.com/apache/superset/pull/37112#discussion_r2730432597
##########
superset-frontend/src/dashboard/components/SliceHeader/SliceHeader.test.tsx:
##########
@@ -677,8 +671,187 @@ test('Should show RowCountLabel in embedded when uiConfig.showRowLimitWarning is
initialState: rowCountState,
});
- expect(screen.getByTestId('warning')).toBeInTheDocument();
+ expect(screen.queryByTestId('warning')).not.toBeInTheDocument();
+ expect(screen.queryByTestId('row-count-label')).not.toBeInTheDocument();
mockIsEmbedded.mockRestore();
mockUseUiConfig.mockRestore();
});
+
+test('Should show row count badge for table chart without server pagination', () => {
+ const mockUseUiConfig = useUiConfig as jest.MockedFunction<
+ typeof useUiConfig
+ >;
+ mockUseUiConfig.mockReturnValue({
+ hideTitle: false,
+ hideTab: false,
+ hideNav: false,
+ hideChartControls: false,
+ emitDataMasks: false,
+ showRowLimitWarning: true,
+ });
+
+ const props = createProps({
+ formData: {
+ ...createProps().formData,
+ viz_type: VizType.Table,
+ row_limit: 10,
+ },
+ slice: {
+ ...createProps().slice,
+ form_data: {
+ ...createProps().slice.form_data,
+ viz_type: VizType.Table,
+ row_limit: 10,
+ },
+ viz_type: VizType.Table,
+ },
Review Comment:
You're right - we shouldn't single out table charts. I've removed that restriction.
The row limit warning now shows for all chart types when the limit is reached (restoring the original behavior from your PR that you mentioned before).
The table-specific logic (isTableChart check) only applies to how we get the row count:
Table charts with server pagination: Use the total count from the second query (the fix for server pagination)
All other charts: Use sql_rowcount from the first query (original behavior)
The warning itself is no longer restricted to table charts - it works for bar charts, line charts, etc., just as you originally implemented.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: charts row limit warning is missing for server [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #37112:
URL: https://github.com/apache/superset/pull/37112#discussion_r2731813907
##########
superset-frontend/src/dashboard/components/SliceHeader/SliceHeader.test.tsx:
##########
@@ -600,11 +626,25 @@ test('Should render RowCountLabel when row limit is hit, and hide it otherwise',
);
expect(screen.queryByTestId('warning')).not.toBeInTheDocument();
+
+ mockUseUiConfig.mockRestore();
});
Review Comment:
**Suggestion:** The tests call `mockUseUiConfig.mockRestore()`, but `useUiConfig` was mocked with `jest.fn()` at module scope, and `mockRestore` is not available on plain jest mocks (it's only available on spies created by `jest.spyOn`). Calling `mockRestore()` will throw a TypeError at runtime. Restore the mock to the module default by calling `mockUseUiConfig.mockReturnValue(...)` with the original default config instead. [possible bug]
<details>
<summary><b>Severity Level:</b> Critical π¨</summary>
```mdx
- β SliceHeader.test.tsx fails with TypeError in CI.
- β οΈ CI pipeline flags failing unit tests.
- β οΈ Developers blocked merging by failing tests.
```
</details>
```suggestion
mockUseUiConfig.mockReturnValue({
hideTitle: false,
hideTab: false,
hideNav: false,
hideChartControls: false,
emitDataMasks: false,
showRowLimitWarning: false,
});
```
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Run the SliceHeader unit tests: execute Jest for this file, e.g. `yarn test
superset-frontend/src/dashboard/components/SliceHeader/SliceHeader.test.tsx -t "Should
render RowCountLabel when row limit is hit, and hide it otherwise"`. This loads the test
module and executes the test body starting at
`superset-frontend/src/dashboard/components/SliceHeader/SliceHeader.test.tsx:568` (test
"Should render RowCountLabel when row limit is hit, and hide it otherwise").
2. Inside that test, the local typed alias is created and the mock is overridden at
`...SliceHeader.test.tsx:601` (`const mockUseUiConfig = useUiConfig as
jest.MockedFunction<typeof useUiConfig>; mockUseUiConfig.mockReturnValue({...})`), setting
a per-test return value for the module-scoped jest.fn() mock.
3. At the end of the test the code calls `mockUseUiConfig.mockRestore()` at
`superset-frontend/src/dashboard/components/SliceHeader/SliceHeader.test.tsx:631`. Because
`useUiConfig` was mocked at module scope with `jest.fn()` (via
`jest.mock('src/components/UiConfigContext', () => ({ useUiConfig:
jest.fn().mockReturnValue(...) }))`), the resulting `mockUseUiConfig` is a plain jest.fn()
spy, which does not implement `mockRestore()`. Jest will raise a TypeError
("mockUseUiConfig.mockRestore is not a function") when that line is executed.
4. Observable failure: the single test run or the whole test suite will crash/fail with a
TypeError at the call site (`...SliceHeader.test.tsx:631`), causing CI or local test runs
to show a failing test run. The correct way to restore the module-level mock is to call
`mockUseUiConfig.mockReturnValue(originalDefault)` or use `jest.resetAllMocks()` /
`jest.restoreAllMocks()` depending on mocking approach. If the existing pattern (module
mock via jest.fn()) is intentional, the `mockRestore()` call is a bug and must be replaced
as suggested.
```
</details>
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/SliceHeader/SliceHeader.test.tsx
**Line:** 631:631
**Comment:**
*Possible Bug: The tests call `mockUseUiConfig.mockRestore()`, but `useUiConfig` was mocked with `jest.fn()` at module scope, and `mockRestore` is not available on plain jest mocks (it's only available on spies created by `jest.spyOn`). Calling `mockRestore()` will throw a TypeError at runtime. Restore the mock to the module default by calling `mockUseUiConfig.mockReturnValue(...)` with the original default config instead.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: charts row limit warning is missing for server [sc-89835] [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37112:
URL: https://github.com/apache/superset/pull/37112#issuecomment-3747410561
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: charts row limit warning is missing for server [superset]
Posted by "ramiroaquinoromero (via GitHub)" <gi...@apache.org>.
ramiroaquinoromero commented on code in PR #37112:
URL: https://github.com/apache/superset/pull/37112#discussion_r2730428929
##########
superset-frontend/src/dashboard/components/SliceHeader/SliceHeader.test.tsx:
##########
@@ -565,7 +565,7 @@ test('Add extension to SliceHeader', () => {
expect(screen.getByText('This is an extension')).toBeInTheDocument();
});
-test('Should render RowCountLabel when row limit is hit, and hide it otherwise', () => {
+test('Should NOT render warning for non-table chart when row limit is hit', () => {
Review Comment:
Updated
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: charts row limit warning is missing for server [superset]
Posted by "ramiroaquinoromero (via GitHub)" <gi...@apache.org>.
ramiroaquinoromero commented on code in PR #37112:
URL: https://github.com/apache/superset/pull/37112#discussion_r2730310575
##########
superset-frontend/src/dashboard/components/SliceHeader/index.tsx:
##########
@@ -304,18 +316,20 @@ const SliceHeader = forwardRef<HTMLDivElement, SliceHeaderProps>(
<FiltersBadge chartId={slice.slice_id} />
)}
- {shouldShowRowLimitWarning && sqlRowCount === rowLimit && (
+ {showRowCountLabel && (
<RowCountLabel
rowcount={sqlRowCount}
limit={rowLimit}
label={
- <Icons.WarningOutlined
- iconSize="l"
- iconColor={theme.colorWarning}
- css={theme => css`
- padding: ${theme.sizeUnit}px;
- `}
- />
+ showRowLimitWarning ? (
+ <Icons.WarningOutlined
+ iconSize="l"
+ iconColor={theme.colorWarning}
+ css={theme => css`
+ padding: ${theme.sizeUnit}px;
+ `}
+ />
+ ) : undefined
Review Comment:
Good morning @msyavuz this is a good point. However, the logic needs to stay in SliceHeader because:
RowCountLabel is a generic component - it doesn't know about server pagination or how to extract the total count from the second query response (lines 198-206). This extraction logic is the actual fix for the bug.
The showRowLimitWarning check determines the visual presentation - it decides whether to show just the warning icon or the default row count label. RowCountLabel's internal limitReached logic then wraps whatever we pass with the tooltip.
Moving this logic into RowCountLabel would require passing queriesResponse, formData, and chart-specific context, making it tightly coupled to dashboard internals. It's currently also used in SqlLab where this logic wouldn't apply.
The key fix is passing the correct sqlRowCount (from the second query when server pagination is enabled) to RowCountLabel, which already handles the tooltip logic correctly.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: charts row limit warning is missing for server [superset]
Posted by "ramiroaquinoromero (via GitHub)" <gi...@apache.org>.
ramiroaquinoromero commented on PR #37112:
URL: https://github.com/apache/superset/pull/37112#issuecomment-3804979975
> Here's the actual review comment #33781 (comment). Can we check if this is not causing any rerenders?
Good catch! I've optimized the selector implementation to prevent unnecessary rerenders.
Current implementation (lines 189-208):
```
const firstQueryResponse = useSelector<RootState, QueryData | undefined>(
state => state.charts[slice.slice_id].queriesResponse?.[0],
);
const secondQueryResponse = useSelector<RootState, QueryData | undefined>(
state => state.charts[slice.slice_id].queriesResponse?.[1],
);
```
Why this avoids rerenders:
Each useSelector only subscribes to a specific query index ([0] or [1])
Changes to unrelated queries won't trigger rerenders
Only rerenders when the specific query response we're using actually changes
This follows the same pattern from PR #33781 where you separated the selectors to avoid the array reference changing unnecessarily.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: charts row limit warning is missing for server [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37112:
URL: https://github.com/apache/superset/pull/37112#issuecomment-3805266093
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: charts row limit warning is missing for server [sc-89835] [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #37112:
URL: https://github.com/apache/superset/pull/37112#issuecomment-3747416376
### <span aria-hidden="true">β
</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">π¨</span> Latest commit | 5f71a00fe5fb0fa18fc8dc6092958575435a29a8 |
|<span aria-hidden="true">π</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6966fffe06887b0008f3e2cd |
|<span aria-hidden="true">π</span> Deploy Preview | [https://deploy-preview-37112--superset-docs-preview.netlify.app](https://deploy-preview-37112--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">π±</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-37112--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: charts row limit warning is missing for server [superset]
Posted by "ramiroaquinoromero (via GitHub)" <gi...@apache.org>.
ramiroaquinoromero commented on PR #37112:
URL: https://github.com/apache/superset/pull/37112#issuecomment-3803247314
Good morning @msyavuz related to this comment.
https://github.com/apache/superset/pull/37112#discussion_r2704009010
This is a good point, but both approaches are functionally equivalent for preventing false warnings when no limit is set:
Your PR: rowLimit = Number(formData.row_limit || -1) β sqlRowCount === -1
My change: rowLimit = Number(formData.row_limit ?? 0) β rowLimit > 0 check prevents warning
No regression occurs because the rowLimit > 0 guard serves the same purpose as your -1 sentinel value.
Additional improvement: I changed the comparison from === to >= because the warning should show when the limit is reached or exceeded, not just when exactly equal. For example, if a query returns 101 rows with a limit of 100, we should still show the warning.
If you prefer to keep the -1 pattern for consistency with your original implementation, I can update it to:
`const rowLimit = Number(formData.row_limit || -1);`
Please let me know I can update this.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: charts row limit warning is missing for server [superset]
Posted by "ramiroaquinoromero (via GitHub)" <gi...@apache.org>.
ramiroaquinoromero commented on PR #37112:
URL: https://github.com/apache/superset/pull/37112#issuecomment-3804981772
@msyavuz Please review it.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: charts row limit warning is missing for server [superset]
Posted by "ramiroaquinoromero (via GitHub)" <gi...@apache.org>.
ramiroaquinoromero commented on PR #37112:
URL: https://github.com/apache/superset/pull/37112#issuecomment-3803323275
@msyavuz Please review it.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: charts row limit warning is missing for server [sc-89835] [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #37112:
URL: https://github.com/apache/superset/pull/37112#discussion_r2700646548
##########
superset-frontend/src/dashboard/components/SliceHeader/SliceHeader.test.tsx:
##########
@@ -677,8 +671,187 @@ test('Should show RowCountLabel in embedded when uiConfig.showRowLimitWarning is
initialState: rowCountState,
});
- expect(screen.getByTestId('warning')).toBeInTheDocument();
+ expect(screen.queryByTestId('warning')).not.toBeInTheDocument();
+ expect(screen.queryByTestId('row-count-label')).not.toBeInTheDocument();
mockIsEmbedded.mockRestore();
mockUseUiConfig.mockRestore();
});
+
+test('Should show row count badge for table chart without server pagination', () => {
+ const mockUseUiConfig = useUiConfig as jest.MockedFunction<
+ typeof useUiConfig
+ >;
+ mockUseUiConfig.mockReturnValue({
+ hideTitle: false,
+ hideTab: false,
+ hideNav: false,
+ hideChartControls: false,
+ emitDataMasks: false,
+ showRowLimitWarning: true,
+ });
+
+ const props = createProps({
+ formData: {
+ ...createProps().formData,
+ viz_type: VizType.Table,
+ row_limit: 10,
+ },
+ slice: {
+ ...createProps().slice,
+ form_data: {
+ ...createProps().slice.form_data,
+ viz_type: VizType.Table,
+ row_limit: 10,
+ },
+ viz_type: VizType.Table,
+ },
+ });
+ const tableState = {
+ ...initialState,
+ charts: {
+ [props.slice.slice_id]: {
+ id: MOCKED_CHART_ID,
+ chartStatus: 'rendered',
+ queriesResponse: [
+ {
+ sql_rowcount: 50,
+ data: [],
+ },
+ ],
+ },
+ },
+ };
+
+ render(<SliceHeader {...props} />, {
+ useRedux: true,
+ useRouter: true,
+ initialState: tableState,
+ });
+
+ expect(screen.getByTestId('warning')).toBeInTheDocument();
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing testId on warning icon</b></div>
<div id="fix">
Add `data-testid="warning"` to the Icons.WarningOutlined element to satisfy the testβs `getByTestId('warning')` query.
</div>
</div>
<small><i>Code Review Run #86d858</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/dashboard/components/SliceHeader/SliceHeader.test.tsx:
##########
@@ -677,8 +671,187 @@ test('Should show RowCountLabel in embedded when uiConfig.showRowLimitWarning is
initialState: rowCountState,
});
- expect(screen.getByTestId('warning')).toBeInTheDocument();
+ expect(screen.queryByTestId('warning')).not.toBeInTheDocument();
+ expect(screen.queryByTestId('row-count-label')).not.toBeInTheDocument();
mockIsEmbedded.mockRestore();
mockUseUiConfig.mockRestore();
});
+
+test('Should show row count badge for table chart without server pagination', () => {
+ const mockUseUiConfig = useUiConfig as jest.MockedFunction<
+ typeof useUiConfig
+ >;
+ mockUseUiConfig.mockReturnValue({
+ hideTitle: false,
+ hideTab: false,
+ hideNav: false,
+ hideChartControls: false,
+ emitDataMasks: false,
+ showRowLimitWarning: true,
+ });
+
+ const props = createProps({
+ formData: {
+ ...createProps().formData,
+ viz_type: VizType.Table,
+ row_limit: 10,
+ },
+ slice: {
+ ...createProps().slice,
+ form_data: {
+ ...createProps().slice.form_data,
+ viz_type: VizType.Table,
+ row_limit: 10,
+ },
+ viz_type: VizType.Table,
+ },
+ });
+ const tableState = {
+ ...initialState,
+ charts: {
+ [props.slice.slice_id]: {
+ id: MOCKED_CHART_ID,
+ chartStatus: 'rendered',
+ queriesResponse: [
+ {
+ sql_rowcount: 50,
+ data: [],
+ },
+ ],
+ },
+ },
+ };
+
+ render(<SliceHeader {...props} />, {
+ useRedux: true,
+ useRouter: true,
+ initialState: tableState,
+ });
+
+ expect(screen.getByTestId('warning')).toBeInTheDocument();
+
+ mockUseUiConfig.mockRestore();
+});
+
+test('Should show row count warning for table chart with server pagination when limit is reached', () => {
+ const props = createProps({
+ formData: {
+ ...createProps().formData,
+ viz_type: VizType.Table,
+ row_limit: 10,
+ server_pagination: true,
+ },
+ slice: {
+ ...createProps().slice,
+ form_data: {
+ ...createProps().slice.form_data,
+ viz_type: VizType.Table,
+ row_limit: 10,
+ server_pagination: true,
+ },
+ viz_type: VizType.Table,
+ },
+ });
+ const tableWithPaginationState = {
+ ...initialState,
+ charts: {
+ [props.slice.slice_id]: {
+ id: MOCKED_CHART_ID,
+ chartStatus: 'rendered',
+ queriesResponse: [
+ {
+ sql_rowcount: 10,
+ data: Array(10).fill({}),
+ },
+ {
+ data: [{ rowcount: 50 }],
+ },
+ ],
+ },
+ },
+ };
+
+ const mockUseUiConfig = useUiConfig as jest.MockedFunction<
+ typeof useUiConfig
+ >;
+ mockUseUiConfig.mockReturnValue({
+ hideTitle: false,
+ hideTab: false,
+ hideNav: false,
+ hideChartControls: false,
+ emitDataMasks: false,
+ showRowLimitWarning: true,
+ });
+
+ render(<SliceHeader {...props} />, {
+ useRedux: true,
+ useRouter: true,
+ initialState: tableWithPaginationState,
+ });
+
+ expect(screen.getByTestId('warning')).toBeInTheDocument();
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing testId on warning icon</b></div>
<div id="fix">
Add `data-testid="warning"` to the Icons.WarningOutlined element to satisfy the testβs `getByTestId('warning')` query.
</div>
</div>
<small><i>Code Review Run #86d858</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: charts row limit warning is missing for server [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #37112:
URL: https://github.com/apache/superset/pull/37112#discussion_r2730433635
##########
superset-frontend/src/dashboard/components/SliceHeader/SliceHeader.test.tsx:
##########
@@ -600,11 +626,69 @@ test('Should render RowCountLabel when row limit is hit, and hide it otherwise',
);
expect(screen.queryByTestId('warning')).not.toBeInTheDocument();
+
+ mockUseUiConfig.mockRestore();
Review Comment:
**Suggestion:** Calling `mockUseUiConfig.mockRestore()` on a mock created via `jest.fn()` will throw because `mockRestore` is only available on spies created with `jest.spyOn()`; restore the original test default by resetting the mock's return value instead. [possible bug]
<details>
<summary><b>Severity Level:</b> Critical π¨</summary>
```mdx
- β SliceHeader unit test fails with runtime TypeError.
- β CI job running Jest will fail this test suite.
- β οΈ Developers blocked by failing local test runs.
```
</details>
```suggestion
mockUseUiConfig.mockReturnValue({
hideTitle: false,
hideTab: false,
hideNav: false,
hideChartControls: false,
emitDataMasks: false,
showRowLimitWarning: false,
});
```
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Run the test file
`superset-frontend/src/dashboard/components/SliceHeader/SliceHeader.test.tsx` (e.g. `jest
SliceHeader.test.tsx`).
2. In the test setup the mock is created/typed at lines 601-604:
- `601 const mockUseUiConfig = useUiConfig as jest.MockedFunction<`
- `602 typeof useUiConfig`
- `603 >;`
- `604 mockUseUiConfig.mockReturnValue({ ... });`
This shows `useUiConfig` is a `jest.fn()` mock, not a spy.
3. Later the test calls the line at 630: `630 mockUseUiConfig.mockRestore();`. Because
`mockUseUiConfig` is a plain `jest.fn()` created via `jest.mock(...)`, `mockRestore` is
not defined on it.
4. Expected outcome: running the test will throw a runtime TypeError (e.g.,
"mockUseUiConfig.mockRestore is not a function") at line 630, failing the test run.
Replacing the call with `mockReturnValue(...)` (or `mockReset()`/`mockImplementationOnce`)
restores intended behavior without throwing.
```
</details>
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/SliceHeader/SliceHeader.test.tsx
**Line:** 630:630
**Comment:**
*Possible Bug: Calling `mockUseUiConfig.mockRestore()` on a mock created via `jest.fn()` will throw because `mockRestore` is only available on spies created with `jest.spyOn()`; restore the original test default by resetting the mock's return value instead.
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/dashboard/components/SliceHeader/SliceHeader.test.tsx:
##########
@@ -632,11 +716,13 @@ test('Should hide RowCountLabel in embedded by default', () => {
});
expect(screen.queryByTestId('warning')).not.toBeInTheDocument();
+ expect(screen.queryByTestId('row-count-label')).not.toBeInTheDocument();
mockIsEmbedded.mockRestore();
Review Comment:
**Suggestion:** Calling `mockIsEmbedded.mockRestore()` on a `jest.fn()` mock will throw because `mockRestore` isn't supported; restore the original behavior by setting the mock return value back to `false`. [possible bug]
<details>
<summary><b>Severity Level:</b> Critical π¨</summary>
```mdx
- β SliceHeader embedded-mode tests fail at runtime.
- β CI Jest job will fail this test suite.
- β οΈ Local development interrupted by failing tests.
```
</details>
```suggestion
mockIsEmbedded.mockReturnValue(false);
```
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Run the test file
`superset-frontend/src/dashboard/components/SliceHeader/SliceHeader.test.tsx`.
2. The `isEmbedded` mock is created/typed in this test around line 678:
- `678 const mockIsEmbedded = isEmbedded as jest.MockedFunction<typeof isEmbedded>;`
- The test sets `mockIsEmbedded.mockReturnValue(true);` at ~683.
3. Later the test calls `721 mockIsEmbedded.mockRestore();`. Since `mockIsEmbedded` is a
`jest.fn()` mock (created via `jest.mock(...)`), `mockRestore` is not present.
4. Running the test triggers a TypeError at line 721 ("mockIsEmbedded.mockRestore is not a
function"), causing the test to fail. Resetting the mock via `mockReturnValue(false)` or
`mockReset()` is the correct non-throwing behavior.
```
</details>
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/SliceHeader/SliceHeader.test.tsx
**Line:** 721:721
**Comment:**
*Possible Bug: Calling `mockIsEmbedded.mockRestore()` on a `jest.fn()` mock will throw because `mockRestore` isn't supported; restore the original behavior by setting the mock return value back to `false`.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: charts row limit warning is missing for server [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe merged PR #37112:
URL: https://github.com/apache/superset/pull/37112
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: charts row limit warning is missing for server [sc-89835] [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37112:
URL: https://github.com/apache/superset/pull/37112#issuecomment-3758597238
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: charts row limit warning is missing for server [sc-89835] [superset]
Posted by "ramiroaquinoromero (via GitHub)" <gi...@apache.org>.
ramiroaquinoromero commented on code in PR #37112:
URL: https://github.com/apache/superset/pull/37112#discussion_r2700575173
##########
superset-frontend/src/dashboard/components/SliceHeader/index.tsx:
##########
@@ -186,14 +186,24 @@ const SliceHeader = forwardRef<HTMLDivElement, SliceHeaderProps>(
({ dashboardInfo }) => dashboardInfo.crossFiltersEnabled,
);
- const firstQueryResponse = useSelector<RootState, QueryData | undefined>(
- state => state.charts[slice.slice_id].queriesResponse?.[0],
+ const queriesResponse = useSelector<RootState, QueryData[] | null | undefined>(
+ state => state.charts[slice.slice_id].queriesResponse,
);
const theme = useTheme();
- const rowLimit = Number(formData.row_limit || -1);
- const sqlRowCount = Number(firstQueryResponse?.sql_rowcount || 0);
+ const rowLimit = Number(formData.row_limit ?? 0);
Review Comment:
Haha, thanks! Yeah, I double-checked and it's safe to go. Thanks for the approval and for catching that detail!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: charts row limit warning is missing for server [sc-89835] [superset]
Posted by "ramiroaquinoromero (via GitHub)" <gi...@apache.org>.
ramiroaquinoromero commented on code in PR #37112:
URL: https://github.com/apache/superset/pull/37112#discussion_r2700612010
##########
superset-frontend/src/dashboard/components/SliceHeader/index.tsx:
##########
@@ -304,18 +314,20 @@ const SliceHeader = forwardRef<HTMLDivElement, SliceHeaderProps>(
<FiltersBadge chartId={slice.slice_id} />
)}
- {shouldShowRowLimitWarning && sqlRowCount === rowLimit && (
+ {!uiConfig.hideChartControls && isTableChart && sqlRowCount > 0 && (
Review Comment:
Done!
##########
superset-frontend/src/dashboard/components/SliceHeader/index.tsx:
##########
@@ -304,18 +314,20 @@ const SliceHeader = forwardRef<HTMLDivElement, SliceHeaderProps>(
<FiltersBadge chartId={slice.slice_id} />
)}
- {shouldShowRowLimitWarning && sqlRowCount === rowLimit && (
+ {!uiConfig.hideChartControls && isTableChart && sqlRowCount > 0 && (
<RowCountLabel
rowcount={sqlRowCount}
limit={rowLimit}
label={
- <Icons.WarningOutlined
- iconSize="l"
- iconColor={theme.colorWarning}
- css={theme => css`
- padding: ${theme.sizeUnit}px;
- `}
- />
+ shouldShowRowLimitWarning && sqlRowCount >= rowLimit && rowLimit > 0 ? (
Review Comment:
Done!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org