You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "rusackas (via GitHub)" <gi...@apache.org> on 2025/08/05 19:44:04 UTC
[PR] fix(explore): Reset table pagination when filtering in View as Table [superset]
rusackas opened a new pull request, #34562:
URL: https://github.com/apache/superset/pull/34562
### SUMMARY
Fixes #31403
When using "View as Table" on a Mixed Chart, navigating to the last page and then searching for text not present in the table would cause an error: "getPaginationModel(): currentPage shouldn't be greater than totalPages"
This PR fixes the issue by adding a `key` prop to the TableView component that changes with the filterText. This causes the component to remount and reset pagination to page 0 whenever the filter changes, preventing the pagination state from being out of sync with the filtered data.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before: Error thrown when searching after navigating to last page
After: Pagination resets to page 1 when filtering, no error
### TESTING INSTRUCTIONS
1. Create or open a Mixed Chart (Mixed Time-Series) in Superset
2. Click "View as Table" in the chart options menu
3. Navigate to the last page using pagination controls
4. In the search box, type text that doesn't exist in the table
5. Verify that no error occurs and the table shows "No results"
6. Clear the search box and verify data returns to page 1
### ADDITIONAL INFORMATION
- [x] Has associated issue: Fixes #31403
- [ ] Required feature flags: None
- [x] Changes UI: Minor fix to table pagination behavior
- [ ] Includes DB Migration: No
- [ ] Introduces new feature or API: No
- [ ] Removes existing feature or API: No
🤖 Generated with [Claude Code](https://claude.ai/code)
--
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(TableView): reset pagination when data reduces below current page [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #34562:
URL: https://github.com/apache/superset/pull/34562#issuecomment-3886214961
## **Sequence Diagram**
This PR ensures TableView automatically resets client-side pagination to the first page when a filter/search reduces the number of pages below the current page, preventing react-table pagination errors. The diagram shows the main success path for client-side pagination reset after filtering.
```mermaid
sequenceDiagram
participant User
participant TableView
participant PaginationState
User->>TableView: Type filter / search (client-side)
TableView->>TableView: Apply filter and recalculate pageCount
alt current page > available pages
TableView->>PaginationState: gotoPage(0) (reset to first page)
end
TableView-->>User: Render page 1 (items or "No results")
```
---
*Generated by [CodeAnt AI](https://codeant.ai)*
--
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(explore): Reset table pagination when filtering in View as Table [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #34562:
URL: https://github.com/apache/superset/pull/34562#issuecomment-3862283284
⚠️ **DEPRECATED WORKFLOW** ⚠️
@SBIN2010 This workflow is deprecated! Please use the new **Superset Showtime** system instead:
- Replace "testenv-up" label with "🎪 trigger-start"
- Better reliability and easier management
- See https://github.com/mistercrunch/superset-showtime for details
Processing your ephemeral environment request [here](https://github.com/apache/superset/actions/runs/21763801406). Action: **up**. More information on [how to use or configure ephemeral environments](https://superset.apache.org/docs/contributing/howtos/#github-ephemeral-environments)
--
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(TableView): reset pagination when data reduces below current page [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #34562:
URL: https://github.com/apache/superset/pull/34562#discussion_r3042448646
##########
superset-frontend/src/explore/components/DataTablesPane/test/SingleQueryResultPane.test.tsx:
##########
@@ -0,0 +1,110 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import {
+ render,
+ screen,
+ waitFor,
+ userEvent,
+} from 'spec/helpers/testing-library';
+import { GenericDataType } from '@apache-superset/core/api/core';
+import { SingleQueryResultPane } from '../components/SingleQueryResultPane';
+
+// Mock data matching what useFilteredTableData expects: Record<string, any>[]
+const mockData: Record<string, unknown>[] = Array.from(
+ { length: 30 },
+ (_, i) => ({
+ id: i + 1,
+ name: `Item ${i + 1}`,
+ category: i % 2 === 0 ? 'Action' : 'Horror',
+ }),
+);
+
+const defaultProps = {
+ // Note: The SingleQueryResultPaneProp type expects Record<string, any>[][]
+ // but useFilteredTableData and useTableColumns actually work with Record<string, any>[]
+ // This type mismatch exists in the codebase - cast through unknown to satisfy TypeScript
+ data: mockData as unknown as Record<string, unknown>[][],
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Type mismatch in data prop</b></div>
<div id="fix">
The type cast here works around a mismatch where the interface defines data as a nested array, but hooks like `useFilteredTableData` and `useTableColumns` expect a flat array of row objects. This could lead to runtime errors if data structures don't align. Consider fixing the type definition in `QueryResultInterface` for better type safety.
</div>
</div>
<small><i>Code Review Run #149a4f</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(explore): Reset table pagination when filtering in View as Table [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 #34562:
URL: https://github.com/apache/superset/pull/34562#discussion_r2771800601
##########
superset-frontend/src/explore/components/DataTablesPane/components/SingleQueryResultPane.tsx:
##########
@@ -85,6 +87,8 @@ export const SingleQueryResultPane = ({
isPaginationSticky
showRowCount={false}
small
+ initialPageIndex={0}
+ key={filterText}
Review Comment:
**Suggestion:** Using the full `filterText` as the `key` for `TableView` causes the table component to fully unmount and remount on every keystroke while typing in the filter, which repeatedly resets pagination and sorting state and can lead to noticeable performance degradation for large tables, even though a remount is only needed when toggling between filtered and unfiltered states. [performance]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Table view remounts on every filter keystroke.
- ⚠️ Visible UI jank for large datasets while typing.
- ⚠️ Extra work for browser and React reconciliation.
- ❌ Not functionally breaking; prevents pagination error currently.
```
</details>
```suggestion
{/* key changes when filter presence toggles, resetting TableView pagination
only when switching between filtered and unfiltered states to avoid
unnecessary remounts on every keystroke */}
<TableView
columns={columns}
size={TableSize.Small}
data={filteredData}
pageSize={dataSize}
noDataText={t('No results')}
emptyWrapperType={EmptyWrapperType.Small}
className="table-condensed"
isPaginationSticky
showRowCount={false}
small
initialPageIndex={0}
key={filterText ? 'filtered' : 'unfiltered'}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open a Mixed Chart and choose "View as Table" (UI path that renders
SingleQueryResultPane at
superset-frontend/src/explore/components/DataTablesPane/components/SingleQueryResultPane.tsx).
The TableView is rendered in this file (see lines 77-92).
2. Type into the table search box (the input handled by handleInputChange which sets
filterText in this file). Each keystroke updates filterText state (useState at top of the
same file) and re-renders SingleQueryResultPane.
3. Because TableView's React key is the exact filterText (key={filterText} at line 92),
every change to filterText unmounts and remounts TableView. Confirmed by the presence of
key={filterText} on the TableView element in the file.
4. Observe behavior on large tables: each keystroke forces a full TableView remount which
resets internal pagination/sorting and can cause noticeable UI jank or extra work in the
browser (re-render and reinitialization every key press). The reported PR fix prevents
"currentPage > totalPages" by remounting, but using full filterText causes repeated
remounts while typing.
Note: This reproduction is based on the actual component and state usage found in
SingleQueryResultPane.tsx where filterText is the state directly used as the TableView key
(lines 64-92).
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/explore/components/DataTablesPane/components/SingleQueryResultPane.tsx
**Line:** 77:91
**Comment:**
*Performance: Using the full `filterText` as the `key` for `TableView` causes the table component to fully unmount and remount on every keystroke while typing in the filter, which repeatedly resets pagination and sorting state and can lead to noticeable performance degradation for large tables, even though a remount is only needed when toggling between filtered and unfiltered states.
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(explore): Reset table pagination when filtering in View as Table [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #34562:
URL: https://github.com/apache/superset/pull/34562#issuecomment-3857937336
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(explore): Reset table pagination when filtering in View as Table [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #34562:
URL: https://github.com/apache/superset/pull/34562#discussion_r2255191417
##########
superset-frontend/src/explore/components/DataTablesPane/components/SingleQueryResultPane.tsx:
##########
@@ -78,6 +78,8 @@ export const SingleQueryResultPane = ({
isPaginationSticky
showRowCount={false}
small
+ initialPageIndex={0}
+ key={filterText}
Review Comment:
### Improper key usage for forcing re-renders <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Using filterText as a key prop to force table re-render is an anti-pattern and violates React's component design principles.
###### Why this matters
This approach can lead to unnecessary re-renders and potential performance issues. It also breaks React's component state preservation between re-renders.
###### Suggested change ∙ *Feature Preview*
Instead of using filterText as a key, handle the filtering logic within the component or through proper state management:
```typescript
const SingleQueryResultPane = () => {
// ... other code ...
return (
<TableView
columns={columns}
data={filteredData}
// ... other props ...
initialPageIndex={0}
// Remove key={filterText}
/>
);
};
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e6ad97bc-7111-4b99-8f33-09896595bd38/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e6ad97bc-7111-4b99-8f33-09896595bd38?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e6ad97bc-7111-4b99-8f33-09896595bd38?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e6ad97bc-7111-4b99-8f33-09896595bd38?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e6ad97bc-7111-4b99-8f33-09896595bd38)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:858258a3-6844-41eb-ba90-9bf58f7f9801 -->
[](858258a3-6844-41eb-ba90-9bf58f7f9801)
--
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(TableView): reset pagination when data reduces below current page [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #34562:
URL: https://github.com/apache/superset/pull/34562#issuecomment-3863229361
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(TableView): reset pagination when data reduces below current page [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #34562:
URL: https://github.com/apache/superset/pull/34562#issuecomment-4299506201
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(TableView): reset pagination when data reduces below current page [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #34562:
URL: https://github.com/apache/superset/pull/34562#issuecomment-3873819141
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(TableView): reset pagination when data reduces below current page [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #34562:
URL: https://github.com/apache/superset/pull/34562#issuecomment-4195899017
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(TableView): reset pagination when data reduces below current page [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 #34562:
URL: https://github.com/apache/superset/pull/34562#discussion_r3126256092
##########
superset-frontend/src/explore/components/DataTablesPane/test/SingleQueryResultPane.test.tsx:
##########
@@ -0,0 +1,110 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import {
+ render,
+ screen,
+ waitFor,
+ userEvent,
+} from 'spec/helpers/testing-library';
+import { GenericDataType } from '@apache-superset/core/common';
+import { SingleQueryResultPane } from '../components/SingleQueryResultPane';
+
+// Mock data matching what useFilteredTableData expects: Record<string, any>[]
+const mockData: Record<string, unknown>[] = Array.from(
+ { length: 30 },
+ (_, i) => ({
+ id: i + 1,
+ name: `Item ${i + 1}`,
+ category: i % 2 === 0 ? 'Action' : 'Horror',
+ }),
+);
+
+const defaultProps = {
+ // Note: The SingleQueryResultPaneProp type expects Record<string, any>[][]
+ // but useFilteredTableData and useTableColumns actually work with Record<string, any>[]
+ // This type mismatch exists in the codebase - cast through unknown to satisfy TypeScript
+ data: mockData as unknown as Record<string, unknown>[][],
+ colnames: ['id', 'name', 'category'],
+ coltypes: [
+ GenericDataType.Numeric,
+ GenericDataType.String,
+ GenericDataType.String,
+ ],
+ rowcount: 30,
+ datasourceId: '1__table',
+ dataSize: 10, // 10 items per page, so 3 pages total
+ isVisible: true,
+ canDownload: true,
+};
+
+test('SingleQueryResultPane renders table and filters data', async () => {
+ render(<SingleQueryResultPane {...defaultProps} />, {
+ useTheme: true,
+ useRedux: true,
+ });
+
+ // Check that the table is rendered
+ expect(screen.getByText('Item 1')).toBeInTheDocument();
+
+ // Search for 'Action'
+ const searchInput = screen.getByPlaceholderText('Search');
+ await userEvent.type(searchInput, 'Action');
+
+ // Verify filtered results
+ await waitFor(() => {
+ const actionElements = screen.getAllByText('Action');
+ expect(actionElements.length).toBeGreaterThan(0);
+ expect(screen.queryByText('Horror')).not.toBeInTheDocument();
+ });
+});
+
+test('SingleQueryResultPane renders TableView correctly', () => {
+ const { container } = render(<SingleQueryResultPane {...defaultProps} />, {
+ useTheme: true,
+ useRedux: true,
+ });
+
+ // Verify TableView is rendered
+ const tableView = container.querySelector('.table-condensed');
+ expect(tableView).toBeInTheDocument();
+});
+
+test('SingleQueryResultPane shows no results message when filter matches nothing', async () => {
+ render(<SingleQueryResultPane {...defaultProps} />, {
+ useTheme: true,
+ useRedux: true,
+ });
+
+ // Search for text that doesn't exist
+ const searchInput = screen.getByPlaceholderText('Search');
+ await userEvent.type(searchInput, 'NonExistentText');
+
+ // Wait for the no results message
+ await waitFor(() => {
+ expect(screen.getByText('No results')).toBeInTheDocument();
+ });
+
+ // Clear the search
+ await userEvent.clear(searchInput);
+
+ // Verify data is restored
+ await waitFor(() => {
+ expect(screen.getByText('Item 1')).toBeInTheDocument();
+ });
Review Comment:
**Suggestion:** The "returns to page 1" check is currently a false positive because the test never leaves page 1 before clearing the filter. This assertion can succeed even when page reset logic is broken. Navigate to a later page first and assert the active page index after clearing. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Page-reset behavior after filtering not verified by tests.
- ⚠️ Clearing search from last-page scenario remains untested.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In
`superset-frontend/src/explore/components/DataTablesPane/test/SingleQueryResultPane.test.tsx`,
inspect the same test `SingleQueryResultPane shows no results message when filter matches
nothing` at lines 88–110; after asserting `'No results'` is shown, the test clears the
search via `await userEvent.clear(searchInput);` at line 104.
2. Observe the final assertion block at lines 106–108: `// Verify data is restored`
followed by `await waitFor(() => { expect(screen.getByText('Item 1')).toBeInTheDocument();
});`, which treats seeing `Item 1` as evidence that data has returned to "page 1".
3. However, from lines 29–35 we know `mockData` is built with `{ name: 'Item 1' }` as the
first row and `defaultProps` sets `dataSize: 10` at line 51, so on the initial render of
`SingleQueryResultPane` the first page already contains `Item 1`; the test never interacts
with any `TableView` pagination controls rendered in `SingleQueryResultPane.tsx:78–89`, so
the internal `pageIndex` always remains on the first page throughout the test.
4. Because the test never moves away from the first page before clearing the search, the
fact that `Item 1` is visible after `userEvent.clear(searchInput)` does not prove that
pagination was reset correctly from an out-of-range page; this assertion would still pass
even if `TableView` failed to reset `pageIndex` after filtering on a later page, so
regressions in the "return to page 1 when pageIndex exceeds pageCount" logic would go
undetected.
```
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset-frontend%2Fsrc%2Fexplore%2Fcomponents%2FDataTablesPane%2Ftest%2FSingleQueryResultPane.test.tsx%0A%2A%2ALine%3A%2A%2A%20103%3A109%0A%2A%2AComment%3A%2A%2A%0A%09%2ALogic%20Error%3A%20The%20%22returns%20to%20page%201%22%20check%20is%20currently%20a%20false%20positive%20because%20the%20test%20never%20leaves%20page%201%20before%20clearing%20the%20filter.%20This%20assertion%20can%20succeed%20even%20when%20page%20reset%20logic%20is%20broken.%20Navigate%20to%20a%20later%20page%20first%20and%20assert%20the%20active%20page%20index%20after%20clearing.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%2
0the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset-frontend%2Fsrc%2Fexplore%2Fcomponents%2FDataTablesPane%2Ftest%2FSingleQueryResultPane.test.tsx%0A%2A%2ALine%3A%2A%2A%20103%3A109%0A%2A%2AComment%3A%2A%2A%0A%09%2ALogic%20Error%3A%20The%20%22returns%20to%20page%201%22%20check%20is%20currently%20a%20false%20positive%20because%20the%20test%20never%20leaves%20page%201%20before%20clearing%20the%20filter.%20This%20assertion%20can%20succeed%20even%20when%20page%20reset%20logic%20is%20broken.%20Navigate%20to%20a%20later%20page%20first%20and%20assert%20the%20active%20page%20index%20after%20clearing.%0A%0AValidat
e%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(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/explore/components/DataTablesPane/test/SingleQueryResultPane.test.tsx
**Line:** 103:109
**Comment:**
*Logic Error: The "returns to page 1" check is currently a false positive because the test never leaves page 1 before clearing the filter. This assertion can succeed even when page reset logic is broken. Navigate to a later page first and assert the active page index after clearing.
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%2F34562&comment_hash=ac386eea09cb76a6c85db9d513d4fc1897515bccaa5e7d58d1cf25aa97cd7cbc&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F34562&comment_hash=ac386eea09cb76a6c85db9d513d4fc1897515bccaa5e7d58d1cf25aa97cd7cbc&reaction=dislike'>👎</a>
##########
superset-frontend/src/explore/components/DataTablesPane/test/SingleQueryResultPane.test.tsx:
##########
@@ -0,0 +1,110 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import {
+ render,
+ screen,
+ waitFor,
+ userEvent,
+} from 'spec/helpers/testing-library';
+import { GenericDataType } from '@apache-superset/core/common';
+import { SingleQueryResultPane } from '../components/SingleQueryResultPane';
+
+// Mock data matching what useFilteredTableData expects: Record<string, any>[]
+const mockData: Record<string, unknown>[] = Array.from(
+ { length: 30 },
+ (_, i) => ({
+ id: i + 1,
+ name: `Item ${i + 1}`,
+ category: i % 2 === 0 ? 'Action' : 'Horror',
+ }),
+);
+
+const defaultProps = {
+ // Note: The SingleQueryResultPaneProp type expects Record<string, any>[][]
+ // but useFilteredTableData and useTableColumns actually work with Record<string, any>[]
+ // This type mismatch exists in the codebase - cast through unknown to satisfy TypeScript
+ data: mockData as unknown as Record<string, unknown>[][],
+ colnames: ['id', 'name', 'category'],
+ coltypes: [
+ GenericDataType.Numeric,
+ GenericDataType.String,
+ GenericDataType.String,
+ ],
+ rowcount: 30,
+ datasourceId: '1__table',
+ dataSize: 10, // 10 items per page, so 3 pages total
+ isVisible: true,
+ canDownload: true,
+};
+
+test('SingleQueryResultPane renders table and filters data', async () => {
+ render(<SingleQueryResultPane {...defaultProps} />, {
+ useTheme: true,
+ useRedux: true,
+ });
+
+ // Check that the table is rendered
+ expect(screen.getByText('Item 1')).toBeInTheDocument();
+
+ // Search for 'Action'
+ const searchInput = screen.getByPlaceholderText('Search');
+ await userEvent.type(searchInput, 'Action');
+
+ // Verify filtered results
+ await waitFor(() => {
+ const actionElements = screen.getAllByText('Action');
+ expect(actionElements.length).toBeGreaterThan(0);
+ expect(screen.queryByText('Horror')).not.toBeInTheDocument();
+ });
+});
+
+test('SingleQueryResultPane renders TableView correctly', () => {
+ const { container } = render(<SingleQueryResultPane {...defaultProps} />, {
+ useTheme: true,
+ useRedux: true,
+ });
+
+ // Verify TableView is rendered
+ const tableView = container.querySelector('.table-condensed');
+ expect(tableView).toBeInTheDocument();
+});
+
+test('SingleQueryResultPane shows no results message when filter matches nothing', async () => {
+ render(<SingleQueryResultPane {...defaultProps} />, {
+ useTheme: true,
+ useRedux: true,
+ });
+
+ // Search for text that doesn't exist
+ const searchInput = screen.getByPlaceholderText('Search');
+ await userEvent.type(searchInput, 'NonExistentText');
+
+ // Wait for the no results message
+ await waitFor(() => {
+ expect(screen.getByText('No results')).toBeInTheDocument();
+ });
Review Comment:
**Suggestion:** This test never reproduces the reported pagination edge case because it filters immediately without first moving to a later page. As written, it can pass even if pagination reset is broken. Add a step to navigate to the last page before typing the non-matching search term, then assert that pagination resets to page 1. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Pagination regression in View as Table lacks coverage.
- ⚠️ Mixed-chart table search edge case unguarded by tests.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open
`superset-frontend/src/explore/components/DataTablesPane/test/SingleQueryResultPane.test.tsx`
and locate the test `SingleQueryResultPane shows no results message when filter matches
nothing` at lines 88–101, where `SingleQueryResultPane` is rendered with `mockData` (30
rows) and `dataSize: 10` (lines 28–52) to create three pages of data.
2. Note that in this test the only interaction is typing a non-existent term into the
search input via `const searchInput = screen.getByPlaceholderText('Search'); await
userEvent.type(searchInput, 'NonExistentText');` at lines 95–96; there is no click on any
pagination control to move off the first page.
3. Inspect
`superset-frontend/src/explore/components/DataTablesPane/components/SingleQueryResultPane.tsx`:
`TableView` from `@superset-ui/core/components` is rendered with `data={filteredData}` and
`pageSize={dataSize}` at lines 78–83, and `filteredData` is produced by
`useFilteredTableData(filterText, data)` (line 59) from
`DataTableControl/index.tsx:230–253`, so pagination state is managed internally by
`TableView` and depends on the current page index.
4. The production bug described in this PR occurs when the user first navigates to a later
page (e.g., the last page of the three-page dataset via `TableView`'s pagination UI
rendered by `SingleQueryResultPane`) and only then types a filter term that yields zero
matches, leaving `pageIndex` beyond the now-available page range; because the test never
moves off page 1 before filtering, that out-of-range `pageIndex` path is never exercised
and the test will still pass even if the new pagination-reset logic in `TableView` is
removed or broken.
```
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset-frontend%2Fsrc%2Fexplore%2Fcomponents%2FDataTablesPane%2Ftest%2FSingleQueryResultPane.test.tsx%0A%2A%2ALine%3A%2A%2A%2095%3A101%0A%2A%2AComment%3A%2A%2A%0A%09%2ALogic%20Error%3A%20This%20test%20never%20reproduces%20the%20reported%20pagination%20edge%20case%20because%20it%20filters%20immediately%20without%20first%20moving%20to%20a%20later%20page.%20As%20written%2C%20it%20can%20pass%20even%20if%20pagination%20reset%20is%20broken.%20Add%20a%20step%20to%20navigate%20to%20the%20last%20page%20before%20typing%20the%20non-matching%20search%20term%2C%20then%20assert%20that%20pagination%20resets%20to%20page%201.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix
%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset-frontend%2Fsrc%2Fexplore%2Fcomponents%2FDataTablesPane%2Ftest%2FSingleQueryResultPane.test.tsx%0A%2A%2ALine%3A%2A%2A%2095%3A101%0A%2A%2AComment%3A%2A%2A%0A%09%2ALogic%20Error%3A%20This%20test%20never%20reproduces%20the%20reported%20pagination%20edge%20case%20because%20it%20filters%20immediately%20without%20first%20moving%20to%20a%20later%20page.%20As%20written%2C%20it%20can%20pass%20even%20if%20pagination%20reset%20is%20broken.%20Add%20a%20step%20to%20navigate%20to%20the%20last%20page%20befo
re%20typing%20the%20non-matching%20search%20term%2C%20then%20assert%20that%20pagination%20resets%20to%20page%201.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(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/explore/components/DataTablesPane/test/SingleQueryResultPane.test.tsx
**Line:** 95:101
**Comment:**
*Logic Error: This test never reproduces the reported pagination edge case because it filters immediately without first moving to a later page. As written, it can pass even if pagination reset is broken. Add a step to navigate to the last page before typing the non-matching search term, then assert that pagination resets to page 1.
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%2F34562&comment_hash=2b00702486a909bb0b37c24c3fc96683ad7f88048d801df3d68acab4b6ed298f&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F34562&comment_hash=2b00702486a909bb0b37c24c3fc96683ad7f88048d801df3d68acab4b6ed298f&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: 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(TableView): reset pagination when data reduces below current page [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas closed pull request #34562: fix(TableView): reset pagination when data reduces below current page
URL: https://github.com/apache/superset/pull/34562
--
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(TableView): reset pagination when data reduces below current page [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #34562:
URL: https://github.com/apache/superset/pull/34562#issuecomment-4516771320
**Bito Automatic Review Skipped – PR Already Merged**<br/><br/>Bito scheduled an automatic review for this pull request, but the review was skipped because this PR was merged before the review could be run.<br/>No action is needed if you didn't intend to review it. To get a review, you can type `/review` in a comment and save 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(TableView): reset pagination when data reduces below current page [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #34562:
URL: https://github.com/apache/superset/pull/34562#discussion_r3133568118
##########
superset-frontend/src/explore/components/DataTablesPane/test/SingleQueryResultPane.test.tsx:
##########
@@ -0,0 +1,110 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import {
+ render,
+ screen,
+ waitFor,
+ userEvent,
+} from 'spec/helpers/testing-library';
+import { GenericDataType } from '@apache-superset/core/api/core';
+import { SingleQueryResultPane } from '../components/SingleQueryResultPane';
+
+// Mock data matching what useFilteredTableData expects: Record<string, any>[]
+const mockData: Record<string, unknown>[] = Array.from(
+ { length: 30 },
+ (_, i) => ({
+ id: i + 1,
+ name: `Item ${i + 1}`,
+ category: i % 2 === 0 ? 'Action' : 'Horror',
+ }),
+);
+
+const defaultProps = {
+ // Note: The SingleQueryResultPaneProp type expects Record<string, any>[][]
+ // but useFilteredTableData and useTableColumns actually work with Record<string, any>[]
+ // This type mismatch exists in the codebase - cast through unknown to satisfy TypeScript
+ data: mockData as unknown as Record<string, unknown>[][],
Review Comment:
Leaving the cast as-is — the underlying mismatch is in `QueryResultInterface`/`SingleQueryResultPaneProp` (see `types.ts`), which is consumed in several places (`useResultsPane.tsx` etc.) and is out of scope for this pagination fix. The cast is annotated in the test so the next person picking up that refactor has the breadcrumb.
--
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(TableView): reset pagination when data reduces below current page [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #34562:
URL: https://github.com/apache/superset/pull/34562#issuecomment-4303705526
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(explore): Reset table pagination when filtering in View as Table [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #34562:
URL: https://github.com/apache/superset/pull/34562#issuecomment-3172385886
@rusackas Ephemeral environment spinning up at http://35.87.199.83:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup.
--
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(TableView): reset pagination when data reduces below current page [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #34562:
URL: https://github.com/apache/superset/pull/34562#issuecomment-4305935933
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(TableView): reset pagination when data reduces below current page [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #34562:
URL: https://github.com/apache/superset/pull/34562
--
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(explore): Reset table pagination when filtering in View as Table [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #34562:
URL: https://github.com/apache/superset/pull/34562#issuecomment-3862137187
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/21762720158) for [4f9fc93](https://github.com/apache/superset/commit/4f9fc93)
--
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(explore): Reset table pagination when filtering in View as Table [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #34562:
URL: https://github.com/apache/superset/pull/34562#issuecomment-3857801218
## Addressed CodeAnt AI Performance Feedback
Good catch on the performance issue! I've updated the implementation.
### The Problem
Using `key={filterText}` caused the table to remount on **every keystroke** while typing in the filter, which:
- Repeatedly resets pagination and sorting state
- Causes visible UI jank for large datasets
- Creates unnecessary work for React reconciliation
### The Solution
Changed from `key={filterText}` to `key={pageCount}`:
```tsx
key={Math.ceil(filteredData.length / dataSize) || 1}
```
This means the table only remounts when the **number of pages actually changes**, not on every keystroke. This:
- ✅ Still fixes the "currentPage > totalPages" error (the original bug)
- ✅ Avoids unnecessary remounts while typing
- ✅ Better performance for large tables
### Why Not `key={filterText ? 'filtered' : 'unfiltered'}`?
The CodeAnt suggestion of `'filtered' : 'unfiltered'` wouldn't fully fix the bug because:
1. User is on page 3, types "A" → remounts (filtered), goes to page 1 ✓
2. User types "B" (now "AB") → **no remount** because still filtered
3. If "AB" matches fewer items than "A", pagination could still be invalid ✗
Using the page count as the key ensures we remount exactly when needed - when the pagination would become invalid.
--
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(explore): Reset table pagination when filtering in View as Table [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #34562:
URL: https://github.com/apache/superset/pull/34562#issuecomment-3857797637
Rebased on latest master and fixed the TypeScript CI failure:
**Changes:**
- Import `GenericDataType` enum and use proper enum values instead of numeric literals
- Cast `data` through `unknown` to satisfy strict TypeScript checking for the type mismatch that exists in the codebase
The remaining CI errors (in `DatasourceModal`, `FoldersEditor`, `testing-library`) are pre-existing issues in master, not related to this 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(explore): Reset table pagination when filtering in View as Table [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #34562:
URL: https://github.com/apache/superset/pull/34562#issuecomment-3857488603
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(TableView): reset pagination when data reduces below current page [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #34562:
URL: https://github.com/apache/superset/pull/34562#issuecomment-4318426274
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(TableView): reset pagination when data reduces below current page [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #34562:
URL: https://github.com/apache/superset/pull/34562#issuecomment-4503408751
Applied all three review nits in ef6bada7:
1. **DRY** — hoisted `const pageSize = initialPageSize ?? DEFAULT_PAGE_SIZE` above `paginationProps` and reused it in both branches + the deps array. The existing `const pageSize` near the page-reset useEffect is now the same hoisted constant, so the declaration appears once.
2. **Test selector stability** — swapped `container.querySelector('.table-condensed')` for `screen.getByTestId('listview-table')` (the `data-test` attribute is set explicitly on TableCollection).
3. **Documented the type mismatch** — expanded the comment and added a TODO above `data: mockData as unknown as Record<string, unknown>[][]` so `SingleQueryResultPaneProp.data` can be reconciled in a follow-up.
Thanks for the review!
--
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(explore): Reset table pagination when filtering in View as Table [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #34562:
URL: https://github.com/apache/superset/pull/34562#issuecomment-3172382244
@rusackas Processing your ephemeral environment request [here](https://github.com/apache/superset/actions/runs/16857753819). Action: **up**. More information on [how to use or configure ephemeral environments](https://superset.apache.org/docs/contributing/howtos/#github-ephemeral-environments)
--
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(explore): Reset table pagination when filtering in View as Table [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #34562:
URL: https://github.com/apache/superset/pull/34562#issuecomment-3857253684
## **Sequence Diagram**
Shows the fix where changing the table filter causes the TableView to remount (via key={filterText}), resetting pagination to page 1 so filtering can't leave pagination out-of-sync and produce errors.
```mermaid
sequenceDiagram
participant User
participant ChartUI as "Chart UI (View as Table)"
participant TableView as "TableView (paginated)"
participant React as "React (re-mount)"
User->>ChartUI: Open "View as Table"
ChartUI->>TableView: Render table with pagination (page N)
User->>TableView: Navigate to last page
User->>TableView: Enter filter text
TableView->>React: key changes (key=filterText) causing remount
React-->>TableView: New instance with pagination reset to page 0
TableView-->>User: Display filtered results (or "No results")
```
---
*Generated by [CodeAnt AI](https://codeant.ai)*
--
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(explore): Reset table pagination when filtering in View as Table [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #34562:
URL: https://github.com/apache/superset/pull/34562#issuecomment-3857248956
## PR Rebased and Improved
Rebased onto latest master and addressed review comments.
### Review Comment Responses
**1. Korbit AI - "Improper key usage for forcing re-renders"**
The `key` prop approach is actually a [documented React pattern](https://react.dev/learn/you-might-not-need-an-effect#resetting-all-state-when-a-prop-changes) for intentionally resetting component state. From the React docs:
> You can reset a component's state by passing a different key to a component. When the key changes, React re-creates the DOM and resets state for this component.
The TableView component uses `react-table` internally and manages pagination state internally. There's no exposed API to reset pagination from outside. The alternatives would be:
- Modifying TableView (shared component in `@superset-ui/core`) to add a reset mechanism
- Adding controlled pagination to SingleQueryResultPane
The `key` prop is the simplest, most reliable fix for this bug.
**2. Copilot - "Using 'as any' bypasses TypeScript's type safety"**
Fixed. The test now uses proper typing with `Record<string, unknown>[]` and documents the type mismatch that exists in the codebase (the `SingleQueryResultPaneProp` type defines `data` as a 2D array, but `useFilteredTableData` expects a 1D array).
### Other Improvements
- Flattened test structure to use `test()` instead of `describe()` (per project guidelines)
- Added explanatory comment in the component
- Improved test names for clarity
--
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(TableView): reset pagination when data reduces below current page [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #34562:
URL: https://github.com/apache/superset/pull/34562#issuecomment-4301341809
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/34562?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 64.41%. Comparing base ([`135e0f8`](https://app.codecov.io/gh/apache/superset/commit/135e0f809968f14ca4e0ddda3abae7ed1d0eef8c?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`9072faf`](https://app.codecov.io/gh/apache/superset/commit/9072faf389733cf31a1dd22cc9f27f45d5a0251b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 248 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #34562 +/- ##
=======================================
Coverage 64.41% 64.41%
=======================================
Files 2536 2536
Lines 131155 131165 +10
Branches 30450 30457 +7
=======================================
+ Hits 84485 84495 +10
Misses 45207 45207
Partials 1463 1463
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/34562/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [javascript](https://app.codecov.io/gh/apache/superset/pull/34562/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `65.84% <100.00%> (+<0.01%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/34562?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</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(TableView): reset pagination when data reduces below current page [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #34562:
URL: https://github.com/apache/superset/pull/34562#issuecomment-4511373841
Rebased on latest master. The PR scope contracted significantly because #39212 replaced `TableView` with the virtualized `GridTable` in `SingleQueryResultPane`, so the original explore-side fix and its test are no longer applicable (virtualized table doesn't have the "currentPage > totalPages" pagination bug).
What survived the rebase:
- The core `TableView` library fix in `superset-ui-core` — adds a `useEffect` that resets `pageIndex` to 0 when filtering reduces data below the current page.
- The accompanying test `should reset to first page when data reduces below current page`.
Adjustments made during rebase:
- Master uses local `useState` for `pageIndex` (not react-table's `gotoPage`), so I changed `gotoPage(0)` → `setPageIndex(0)` and updated deps.
- Master already hoists `effectivePageSize` above the paginationProps memo, so my earlier review-nit hoisting commit became a no-op and was dropped — I just reuse the existing constant.
The library fix still genuinely benefits any remaining `TableView` callers (and is a sensible default-pagination guarantee), so leaving the PR open. Squashed down to a single clean commit.
--
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(explore): Reset table pagination when filtering in View as Table [superset]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #34562:
URL: https://github.com/apache/superset/pull/34562#discussion_r2257819764
##########
superset-frontend/src/explore/components/DataTablesPane/test/SingleQueryResultPane.test.tsx:
##########
@@ -0,0 +1,103 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import {
+ render,
+ screen,
+ waitFor,
+ userEvent,
+} from 'spec/helpers/testing-library';
+import { SingleQueryResultPane } from '../components/SingleQueryResultPane';
+
+describe('SingleQueryResultPane', () => {
+ const mockData = Array.from({ length: 30 }, (_, i) => ({
+ id: i + 1,
+ name: `Item ${i + 1}`,
+ category: i % 2 === 0 ? 'Action' : 'Horror',
+ }));
+
+ const defaultProps = {
+ data: mockData as any, // Type mismatch between definition and actual usage
Review Comment:
Using 'as any' bypasses TypeScript's type safety. Consider defining proper types or using a more specific type assertion to maintain type safety.
```suggestion
// Define the type for a row of data expected by SingleQueryResultPane
interface DataRow {
id: number;
name: string;
category: string;
}
describe('SingleQueryResultPane', () => {
const mockData: DataRow[] = Array.from({ length: 30 }, (_, i) => ({
id: i + 1,
name: `Item ${i + 1}`,
category: i % 2 === 0 ? 'Action' : 'Horror',
}));
const defaultProps = {
data: mockData,
```
--
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(explore): Reset table pagination when filtering in View as Table [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #34562:
URL: https://github.com/apache/superset/pull/34562#issuecomment-3862313949
@SBIN2010 Ephemeral environment spinning up at http://44.255.205.130:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup.
--
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(TableView): reset pagination when data reduces below current page [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #34562:
URL: https://github.com/apache/superset/pull/34562#issuecomment-3863090602
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | d357454d1cc17772d7a62f98f43a2bcc6b9f1087 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/69868061bd81bb0008251963 |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-34562--superset-docs-preview.netlify.app](https://deploy-preview-34562--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-34562--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(TableView): reset pagination when data reduces below current page [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #34562:
URL: https://github.com/apache/superset/pull/34562#discussion_r2776619213
##########
superset-frontend/packages/superset-ui-core/src/components/TableView/TableView.tsx:
##########
@@ -250,6 +250,22 @@ const RawTableView = ({
}
}, [initialState.sortBy, onServerPagination, serverPagination, sortBy]);
+ // Reset to first page when current page exceeds available pages
+ // (e.g., when filtering reduces the data below the current page)
+ const pageSize = initialPageSize ?? DEFAULT_PAGE_SIZE;
+ const pageCount = Math.ceil(data.length / pageSize);
+ useEffect(() => {
+ if (
+ withPagination &&
+ !serverPagination &&
+ !loading &&
+ pageIndex > pageCount - 1 &&
+ pageCount > 0
+ ) {
+ gotoPage(0);
+ }
+ }, [withPagination, serverPagination, loading, pageIndex, pageCount, gotoPage]);
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Pagination Reset Logic Bug</b></div>
<div id="fix">
The useEffect for resetting pageIndex on data changes has two issues: the condition 'pageIndex > pageCount - 1 && pageCount > 0' fails to reset when pageCount is 0 (e.g., no data), potentially leaving pageIndex invalid. Also, the deps array misses 'data.length' and 'initialPageSize', so the effect won't trigger if data shrinks without changing pageIndex. This could show empty pages incorrectly.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
useEffect(() => {
if (
withPagination &&
!serverPagination &&
!loading &&
pageIndex >= pageCount
) {
gotoPage(0);
}
}, [withPagination, serverPagination, loading, pageIndex, pageCount, gotoPage, data.length, initialPageSize]);
````
</div>
</details>
</div>
<small><i>Code Review Run #586eb2</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(explore): Reset table pagination when filtering in View as Table [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #34562:
URL: https://github.com/apache/superset/pull/34562#issuecomment-3857801391
### Response to CodeAnt AI Performance Suggestion
Thanks for the thoughtful review! I considered the suggested `key={filterText ? 'filtered' : 'unfiltered'}` approach, but it wouldn't fully solve the problem:
**Why the simpler toggle approach doesn't work:**
If you're on page 3 of 3 with filter "Action" and type more to get "ActionX" which yields only 1 page, you'd still get the "currentPage > totalPages" error because the key didn't change ('filtered' → 'filtered').
**Why `key={filterText}` is the right trade-off:**
1. **Correctness**: It prevents the error in all cases
2. **Simplicity**: Avoids adding controlled pagination to the shared TableView component
3. **Acceptable performance**: Tables in View as Table are reasonably sized (paginated results, not huge datasets)
4. **React-idiomatic**: This is a [documented React pattern](https://react.dev/learn/you-might-not-need-an-effect#resetting-all-state-when-a-prop-changes)
I've updated the inline comment to document this trade-off for future maintainers.
--
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(TableView): reset pagination when data reduces below current page [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #34562:
URL: https://github.com/apache/superset/pull/34562#discussion_r3131959773
##########
superset-frontend/src/explore/components/DataTablesPane/components/SingleQueryResultPane.tsx:
##########
@@ -78,6 +78,8 @@ export const SingleQueryResultPane = ({
isPaginationSticky
showRowCount={false}
small
+ initialPageIndex={0}
+ key={filterText}
Review Comment:
Leaving the `QueryResultInterface` cleanup for a follow-up — the pre-existing nested-array vs flat-array mismatch touches `useFilteredTableData`, `useTableColumns`, and callers across the DataTablesPane tree, which is well outside this pagination fix. The comment at lines 39-42 calls it out explicitly so the next pass can pick it up.
##########
superset-frontend/src/explore/components/DataTablesPane/test/SingleQueryResultPane.test.tsx:
##########
@@ -0,0 +1,110 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import {
+ render,
+ screen,
+ waitFor,
+ userEvent,
+} from 'spec/helpers/testing-library';
+import { GenericDataType } from '@apache-superset/core/common';
+import { SingleQueryResultPane } from '../components/SingleQueryResultPane';
+
+// Mock data matching what useFilteredTableData expects: Record<string, any>[]
+const mockData: Record<string, unknown>[] = Array.from(
+ { length: 30 },
+ (_, i) => ({
+ id: i + 1,
+ name: `Item ${i + 1}`,
+ category: i % 2 === 0 ? 'Action' : 'Horror',
+ }),
+);
+
+const defaultProps = {
+ // Note: The SingleQueryResultPaneProp type expects Record<string, any>[][]
+ // but useFilteredTableData and useTableColumns actually work with Record<string, any>[]
+ // This type mismatch exists in the codebase - cast through unknown to satisfy TypeScript
+ data: mockData as unknown as Record<string, unknown>[][],
+ colnames: ['id', 'name', 'category'],
+ coltypes: [
+ GenericDataType.Numeric,
+ GenericDataType.String,
+ GenericDataType.String,
+ ],
+ rowcount: 30,
+ datasourceId: '1__table',
+ dataSize: 10, // 10 items per page, so 3 pages total
+ isVisible: true,
+ canDownload: true,
+};
+
+test('SingleQueryResultPane renders table and filters data', async () => {
+ render(<SingleQueryResultPane {...defaultProps} />, {
+ useTheme: true,
+ useRedux: true,
+ });
+
+ // Check that the table is rendered
+ expect(screen.getByText('Item 1')).toBeInTheDocument();
+
+ // Search for 'Action'
+ const searchInput = screen.getByPlaceholderText('Search');
+ await userEvent.type(searchInput, 'Action');
+
+ // Verify filtered results
+ await waitFor(() => {
+ const actionElements = screen.getAllByText('Action');
+ expect(actionElements.length).toBeGreaterThan(0);
+ expect(screen.queryByText('Horror')).not.toBeInTheDocument();
+ });
+});
+
+test('SingleQueryResultPane renders TableView correctly', () => {
+ const { container } = render(<SingleQueryResultPane {...defaultProps} />, {
+ useTheme: true,
+ useRedux: true,
+ });
+
+ // Verify TableView is rendered
+ const tableView = container.querySelector('.table-condensed');
+ expect(tableView).toBeInTheDocument();
+});
+
+test('SingleQueryResultPane shows no results message when filter matches nothing', async () => {
+ render(<SingleQueryResultPane {...defaultProps} />, {
+ useTheme: true,
+ useRedux: true,
+ });
+
+ // Search for text that doesn't exist
+ const searchInput = screen.getByPlaceholderText('Search');
+ await userEvent.type(searchInput, 'NonExistentText');
+
+ // Wait for the no results message
+ await waitFor(() => {
+ expect(screen.getByText('No results')).toBeInTheDocument();
+ });
Review Comment:
Good catch — fixed in 562b6323. The no-results test now navigates to page 3 before typing the non-matching term, so it actually exercises the `pageIndex > pageCount - 1` branch when the filter collapses results to zero from a later page.
##########
superset-frontend/src/explore/components/DataTablesPane/test/SingleQueryResultPane.test.tsx:
##########
@@ -0,0 +1,110 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import {
+ render,
+ screen,
+ waitFor,
+ userEvent,
+} from 'spec/helpers/testing-library';
+import { GenericDataType } from '@apache-superset/core/common';
+import { SingleQueryResultPane } from '../components/SingleQueryResultPane';
+
+// Mock data matching what useFilteredTableData expects: Record<string, any>[]
+const mockData: Record<string, unknown>[] = Array.from(
+ { length: 30 },
+ (_, i) => ({
+ id: i + 1,
+ name: `Item ${i + 1}`,
+ category: i % 2 === 0 ? 'Action' : 'Horror',
+ }),
+);
+
+const defaultProps = {
+ // Note: The SingleQueryResultPaneProp type expects Record<string, any>[][]
+ // but useFilteredTableData and useTableColumns actually work with Record<string, any>[]
+ // This type mismatch exists in the codebase - cast through unknown to satisfy TypeScript
+ data: mockData as unknown as Record<string, unknown>[][],
+ colnames: ['id', 'name', 'category'],
+ coltypes: [
+ GenericDataType.Numeric,
+ GenericDataType.String,
+ GenericDataType.String,
+ ],
+ rowcount: 30,
+ datasourceId: '1__table',
+ dataSize: 10, // 10 items per page, so 3 pages total
+ isVisible: true,
+ canDownload: true,
+};
+
+test('SingleQueryResultPane renders table and filters data', async () => {
+ render(<SingleQueryResultPane {...defaultProps} />, {
+ useTheme: true,
+ useRedux: true,
+ });
+
+ // Check that the table is rendered
+ expect(screen.getByText('Item 1')).toBeInTheDocument();
+
+ // Search for 'Action'
+ const searchInput = screen.getByPlaceholderText('Search');
+ await userEvent.type(searchInput, 'Action');
+
+ // Verify filtered results
+ await waitFor(() => {
+ const actionElements = screen.getAllByText('Action');
+ expect(actionElements.length).toBeGreaterThan(0);
+ expect(screen.queryByText('Horror')).not.toBeInTheDocument();
+ });
+});
+
+test('SingleQueryResultPane renders TableView correctly', () => {
+ const { container } = render(<SingleQueryResultPane {...defaultProps} />, {
+ useTheme: true,
+ useRedux: true,
+ });
+
+ // Verify TableView is rendered
+ const tableView = container.querySelector('.table-condensed');
+ expect(tableView).toBeInTheDocument();
+});
+
+test('SingleQueryResultPane shows no results message when filter matches nothing', async () => {
+ render(<SingleQueryResultPane {...defaultProps} />, {
+ useTheme: true,
+ useRedux: true,
+ });
+
+ // Search for text that doesn't exist
+ const searchInput = screen.getByPlaceholderText('Search');
+ await userEvent.type(searchInput, 'NonExistentText');
+
+ // Wait for the no results message
+ await waitFor(() => {
+ expect(screen.getByText('No results')).toBeInTheDocument();
+ });
+
+ // Clear the search
+ await userEvent.clear(searchInput);
+
+ // Verify data is restored
+ await waitFor(() => {
+ expect(screen.getByText('Item 1')).toBeInTheDocument();
+ });
Review Comment:
Already addressed in 5c080137 — I dropped the clear-filter assertion entirely rather than try to make it meaningful, since the dedicated `resets pagination when filter reduces data below current page` test covers the reset behavior end-to-end from page 3.
--
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