EnxDev commented on PR #43296:
URL: https://github.com/apache/superset/pull/43296#issuecomment-5339688982
## EnxDev's Review Agent โ apache/superset#43296 ยท HEAD 74efbb7d
**comment** โ the fix is right and the removed guard is properly guarded by
a new test, but the warning text it now surfaces says "the chart" in a pane
that has no chart, and the samples half of the fix stops working above 1k rows.
Reviewed the diff against `RowCountLabel`, `SamplesPane`, `useResultsPane`,
`DrillBy/useResultsTableView` and the samples backend. Behavioral surface of
dropping the `(!onRowLimitChange || rowcount < (rowLimit ?? Infinity))` guard:
Results pane and Samples pane change (intended), DrillBy is unaffected (it
passes neither `rowLimit` nor `effectiveRowLimit`, so `limit` stays `undefined`
and no warning can trigger), and both error branches render the label exactly
as before.
### ๐ด Functional
- **`DataTableControls.tsx:122-126`** ยท _High_ โ The Samples pane defaults
to `rowLimit` 100 (`SamplesPane.tsx:56`) and its `rowcount` is the returned
page size, so on any dataset with โฅ100 rows `rowcount >= limit` is now true and
the label renders red with `RowCountLabel`'s fixed tooltip: *"The row limit set
for the chart was reached. The chart may show partial data."* There is no chart
in the Samples pane, and the truncation came from the pane's own selector. The
same mis-attribution hits Results whenever the dropdown โ not the chart's
`row_limit` โ is the binding limit (chart `row_limit` 10000, selector 100).
This message was unreachable in both panes before the PR, so it is newly
user-visible, and `test/DataTableControls.test.tsx:57` currently locks it in.
Add an optional `limitMessage` prop to `RowCountLabel` and pass a table-neutral
string ("The row limit was reached. The table may show partial data.") from
`TableControls`. **regression test:** render `TableControls
` with `rowcount === rowLimit` and assert the tooltip does not claim the
chart's limit was reached.
### ๐ก Should-fix
- **`SamplesPane.tsx:181`** โ Samples still passes only `rowLimit`, and
`get_limit_clause` resets any `per_page` above `SAMPLES_ROW_LIMIT` (1000,
`superset/config.py:180`) back down to it
(`superset/views/datasource/utils.py:47-49`). Pick 5k or 10k and the backend
returns 1000 rows, `1000 >= 5000` is false, no warning โ the exact
silent-truncation this PR fixes for Results. The new `prefix={t('Limit')}`
makes it worse by asserting an applied limit that was overridden. The samples
response already carries `total_count`; `effectiveRowLimit`/truncation can be
derived from it without plumbing the config to the client.
- **`useResultsPane.tsx:239`** โ The `effectiveRowLimit={effectiveRowLimit}`
plumbing has no test. All five new tests drive `TableControls` directly, so
deleting this line (and the `SingleQueryResultPane` pass-through) leaves CI
green while restoring the "chart `row_limit` truncates silently" case the
description calls out. `test/useResultsPane.test.tsx` already exists โ add a
case with `row_limit` below the selected limit.
- **`test/DataTableControls.test.tsx:66`** โ `queryByRole('tooltip')` runs
synchronously right after `userEvent.hover`, and antd mounts the tooltip after
`mouseEnterDelay`, so this passes whether or not the warning state is correct.
Assert the label is not error-styled instead, or wrap in a short `waitFor`.
### ๐ต Nits
- `DataTableControls.tsx:118` โ `min-width: 160px` is a hardcoded dimension
on a touched line; `theme.sizeUnit * 40`.
- CI is red at HEAD but not because of this diff: `sharded-jest-tests (3)`
fails on `ExploreChartHeader โบ Should open all data download submenu` and
`playwright-tests (chromium)` on `recently-archived.spec.ts`. The code commit
`5bfc52b` was fully green; both appeared only on the master-merge commit
`74efbb7d`, and `ExploreChartHeader.test.tsx` passes locally on master (40/40,
70s โ slow enough to lose the race on CI). Re-run rather than chase.
### ๐ Praise
- `useResultsPane.tsx:239` + `test/DataTableControls.test.tsx:69` โ passing
`min(selected, chart row_limit)` rather than the dropdown value is the
non-obvious half of this fix, and it is the one case with a dedicated test.
<!-- enxdev-review-agent:74efbb7d -->
_Reviewed by EnxDev's Review Agent โ @EnxDev ยท HEAD 74efbb7d._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
