You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "mikebridge (via GitHub)" <gi...@apache.org> on 2026/09/19 22:53:45 UTC
[PR] feat(semantic-layer): gate Table pagination on row offset support [superset]
mikebridge opened a new pull request, #44452:
URL: https://github.com/apache/superset/pull/44452
### SUMMARY
Semantic Table charts can offer Server pagination even when their provider rejects nonzero offsets. This adds the optional SemanticViewFeature.ROW_OFFSET contract (non-negative offset with a positive finite limit) and disables the Table control with an accessible explanation when a semantic view does not advertise it.
Saved pagination settings remain unchanged until an explicit edit. A saved enabled control offers a keyboard-accessible **Turn off server pagination** action. SQL datasets keep their existing controls; direct callers retain provider-local validation.
[SC-121436](https://app.shortcut.com/preset/story/121436). This is PR A: core contract and host controls. The coordinated shell pin/Cube declaration follows separately after merge; MetricFlow pagination remains separate work.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before: unsupported semantic views expose an enabled Server pagination control.
After: the control remains visible but disabled with an explanation; saved enabled settings offer an explicit turn-off action.
Browser/screenshots are deferred to the campaign stack owner; this PR remains draft.
### TESTING INSTRUCTIONS
Local final checks:
- Python models/mapper/core contract suites: **298 passed**.
- Table control/buildQuery and CheckboxControl Jest suites: **63 passed**.
- Red-first evidence: missing enum/metadata contract tests failed; unsupported-control and saved-state tests failed before implementation.
- Required staged hooks and explicit MyPy passed except the normal frontend type hook, which reports TS6305 for unbuilt workspace declarations. The approved strict source-resolved TypeScript check of all four changed TS/TSX files passed with **zero diagnostics**. No type-checking strictness options were relaxed.
- Existing dependencies were reused locally; clean-install/build evidence remains for CI.
Manual follow-up:
1. With semantic layers enabled, open a Table backed by a view without ROW_OFFSET; verify the visible disabled checkbox and explanation.
2. Open a saved chart with server pagination enabled. Confirm its settings/query offsets are not normalized and provider errors remain actionable. Tab to **Turn off server pagination**, activate it, then save explicitly.
3. Switch between supported and unsupported datasources; confirm no automatic pagination-state rewrite.
4. Verify a view declaring ROW_OFFSET and an ordinary SQL dataset retain editable controls.
### ADDITIONAL INFORMATION
- [x] Has associated issue: SC-121436
- [x] Required feature flags: existing SEMANTIC_LAYERS for semantic views; defaults unchanged
- [x] Changes UI
- [ ] Includes DB Migration (follow approval process in SIP-59)
- [x] Introduces new feature or API: optional ROW_OFFSET enum member
- [ ] Removes existing feature or API
No provider implementation, host-wide offset validator, persistence migration or chart rewrite. Missing capability metadata fails closed for the UI. Providers must use a compatible core version before importing the new enum member.
--
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] feat(semantic-layer): gate Table pagination on row offset support [superset]
Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
mikebridge commented on PR #44452:
URL: https://github.com/apache/superset/pull/44452#issuecomment-6047059907
@rebenitez1802 thanks for the detailed review. Here is what changed for each point, in [29a85fa](https://github.com/apache/superset/commit/29a85fa9977907654922fce150472351f0d7bbba):
- ControlHeader a11y: noninteractive headers now render as plain text, while interactive headers keep keyboard activation.
- Stale-datasource gating: when the datasource types disagree, the pagination gate now follows `form_data`, with tests covering both switch directions.
- Description: the private reference has been scrubbed from the PR description.
- Nits: the optional capability opt-in, provider-owned offset/limit validation, themed feedback, the unlabeled reset, saved AG Grid offsets, and no reset flicker while metadata loads are all done. Saved pagination values remain unchanged until an explicit edit.
[37f79ac](https://github.com/apache/superset/commit/37f79acf4bff009b16d66e16ba969feb900b777d) also updates two consumer tests that still assumed those labels were buttons. Could you take another look when you have a moment?
--
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] feat(semantic-layer): gate Table pagination on row offset support [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #44452:
URL: https://github.com/apache/superset/pull/44452#discussion_r4254415718
##########
superset-frontend/src/explore/components/controls/CheckboxControl.tsx:
##########
@@ -50,24 +58,77 @@ const CheckBoxControlWrapper = styled.div`
export default function CheckboxControl({
value = false,
label,
+ disabled = false,
+ disabledReason,
+ resetLabel,
onChange = () => {},
...restProps
}: CheckboxControlProps): JSX.Element {
+ const explanationId = useId();
+ const checkboxId = useId();
const handleChange = useCallback((): void => {
- onChange(!value);
- }, [onChange, value]);
+ if (!disabled) {
+ onChange(!value);
+ }
+ }, [disabled, onChange, value]);
- const checkbox = <Checkbox onChange={handleChange} checked={!!value} />;
+ const checkbox = (
+ <Checkbox
+ id={checkboxId}
+ onChange={handleChange}
+ checked={!!value}
+ disabled={disabled}
+ aria-describedby={disabled && disabledReason ? explanationId : undefined}
+ />
+ );
+ const explanation = disabled && disabledReason && (
+ <Typography.Text id={explanationId} type="secondary">
+ {disabledReason}
+ </Typography.Text>
+ );
+ const resetButton = disabled && value && resetLabel && (
Review Comment:
Once a saved chart has server pagination turned on and the datasource metadata matches, both control panels still hand the checkbox a `resetLabel`, so the `disabled &&` guard here is the only thing keeping "Turn off server pagination" hidden on a supported or ordinary SQL dataset. The enabled-checked cases (`keeps %s pagination editable` and the `ROW_OFFSET` leg of `saved true survives datasource changes`) never assert the button is absent, so dropping `disabled &&` would still pass every test. Could you assert `queryByRole('button', { name: 'Turn off server pagination' })` is absent for a checked, enabled control with `ROW_OFFSET` (and a `table` datasource)?
##########
superset-frontend/src/explore/components/ControlHeader.tsx:
##########
@@ -130,7 +133,7 @@ const ControlHeader: FC<ControlHeaderProps> = ({
</Tooltip>{' '}
</>
)}
- {renderTrigger && (
+ {hovered && renderTrigger && (
Review Comment:
This moves the bolt icon from always-rendered to hover-only (`hovered && renderTrigger`), but no test covers a header that has both `description` and `renderTrigger` while not hovered. If someone reverts to `renderTrigger &&`, the instant-effect icon shows on every unhovered render-trigger control and the suite still passes. Could you add a `ControlHeader` test with `renderTrigger` + `description`, asserting the bolt is absent at `hovered=false`, present at `hovered=true`, and the description help stays present throughout?
--
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] feat(semantic-layer): gate Table pagination on row offset support [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #44452:
URL: https://github.com/apache/superset/pull/44452#issuecomment-5769990389
Heya @mikebridge, thanks for this, nice test coverage on the offset contract. Bito's flagged a couple of things that look real though (the mapStateToProps staleness and the dangling aria-describedby), worth addressing before this merges.
--
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] feat(semantic-layer): gate Table pagination on row offset support [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #44452:
URL: https://github.com/apache/superset/pull/44452#discussion_r4150119906
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/controlPanel.tsx:
##########
@@ -389,6 +390,14 @@ const config: ControlPanelConfig = {
'Enable server side pagination of results (experimental feature)',
),
default: false,
+ shouldMapStateToProps: () => true,
+ mapStateToProps: state => ({
+ disabled: isServerPaginationUnsupported(state),
+ disabledReason: t(
+ 'This semantic view does not support server pagination.',
+ ),
+ resetLabel: t('Turn off server pagination'),
+ }),
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>duplicated control mapping logic</b></div>
<div id="fix">
The `shouldMapStateToProps: () => true` + `mapStateToProps` blocks for `server_pagination` and `server_page_length` are duplicated verbatim in both `plugin-chart-ag-grid-table` and `plugin-chart-table` controlPanel.tsx. Since `isServerPaginationUnsupported` was already extracted to `@superset-ui/chart-controls`, consider sharing these mapStateToProps blocks too to avoid divergence.
</div>
</div>
<small><i>Code Review Run #79b0cb</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] feat(semantic-layer): gate Table pagination on row offset support [superset]
Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
mikebridge commented on code in PR #44452:
URL: https://github.com/apache/superset/pull/44452#discussion_r4167636655
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/controlPanel.tsx:
##########
@@ -389,6 +390,14 @@ const config: ControlPanelConfig = {
'Enable server side pagination of results (experimental feature)',
),
default: false,
+ shouldMapStateToProps: () => true,
+ mapStateToProps: state => ({
+ disabled: isServerPaginationUnsupported(state),
+ disabledReason: t(
+ 'This semantic view does not support server pagination.',
+ ),
+ resetLabel: t('Turn off server pagination'),
+ }),
Review Comment:
Agreed that these two blocks should share one helper. I would rather keep this PR to the gate itself, so I am deferring the extraction to a follow-up (tracked as sc-124090), which will also fold the duplicated control lookup in the AG Grid test into a shared `findControl(name)` helper. `isServerPaginationUnsupported` is already shared from `@superset-ui/chart-controls`, so the two plugins evaluate the same condition today.
--
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] feat(semantic-layer): gate Table pagination on row offset support [superset]
Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
mikebridge commented on code in PR #44452:
URL: https://github.com/apache/superset/pull/44452#discussion_r4221624646
##########
superset-frontend/packages/superset-ui-chart-controls/src/utils/isServerPaginationUnsupported.ts:
##########
@@ -0,0 +1,40 @@
+/**
+ * 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 { DatasourceType } from '@superset-ui/core';
+import type { ControlPanelState } from '../types';
+
+/** SemanticViewFeature value for providers that support row offsets. */
+export const ROW_OFFSET = 'ROW_OFFSET';
+
+/** Keep semantic pagination disabled until the view declares offset support. */
+export function isServerPaginationUnsupported({
+ datasource,
+ form_data,
+}: ControlPanelState): boolean {
+ const datasourceType =
+ form_data.datasource?.split('__')[1] ?? datasource?.type;
Review Comment:
Good catch, addressed in `8a1d5f0f07` using your UID-match suggestion. Table and AG Grid both handle `cube__orders` correctly with and without ROW_OFFSET. Removing the fix fails four new cases; with it in place, all six UID cases and the four stale-metadata cases pass.
--
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] feat(semantic-layer): gate Table pagination on row offset support [superset]
Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
mikebridge commented on PR #44452:
URL: https://github.com/apache/superset/pull/44452#issuecomment-6083701720
@geido Thanks — that suggestion found a real bug, not just a coverage gap.
Writing the test you asked for showed that a switch between two semantic views of the *same* type kept serving the previous view's capabilities: `isServerPaginationUnsupported` only checked `datasource.type === datasourceType`, which stays true across a same-type switch, so a stale `ROW_OFFSET` from the prior view enabled pagination before the selected view's metadata had arrived. The existing cases all changed datasource *type*, which is exactly why they missed it.
Fixed in the push just now (`5e2b689661`): capability metadata is read only when `datasource.uid === form_data.datasource`, so unmatched metadata now behaves like pending metadata — pagination stays conservatively disabled until the selected view declares offset support.
Tests, three per plugin in `plugin-chart-table/test/controlPanel.test.tsx` and `plugin-chart-ag-grid-table/test/controlPanel.test.tsx`:
- `%s ignores stale offset capability from another semantic view` — your scenario, `1__semantic_view` metadata against a `2__semantic_view` selection, for both `server_pagination` and `server_page_length`; it also asserts the control value is not silently reset.
- `ignores stale offset capability when switching opaque semantic UIDs` — the same thing for provider UIDs that don't encode a parseable type (`cube__orders` → `cube__customers`), which the old code treated as a non-semantic datasource and left pagination enabled.
All six fail against the previous helper. Both control-panel suites are green at this commit (61/61).
One adjacent thing I noticed while verifying, worth your view as a follow-up rather than something for this PR: `resetLabel` is still gated on `state.datasource` merely existing rather than on the UID match, so during that same switch the control is correctly disabled but we also offer "Turn off server pagination" and say "This semantic view does not support server pagination" — both based on the previous view's metadata. The `datasource: null` case is handled, but the stale case isn't. Happy to open a separate issue for it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(semantic-layer): gate Table pagination on row offset support [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #44452:
URL: https://github.com/apache/superset/pull/44452#issuecomment-6007213497
## CodeAnt PR Risk: Low Risk
- The PR appears safe to merge; unsupported semantic-view pagination is disabled without rewriting saved settings, while SQL controls remain unchanged.
- Tests cover capability changes, saved pagination settings, query offsets, and the explicit opt-out action.
**Assessed commit:** `bbdbae91e915`
--
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] feat(semantic-layer): gate Table pagination on row offset support [superset]
Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
mikebridge commented on code in PR #44452:
URL: https://github.com/apache/superset/pull/44452#discussion_r4149740693
##########
superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx:
##########
@@ -60,6 +60,20 @@ import { GenericDataType } from '@apache-superset/core/common';
import { isEmpty, last } from 'lodash-es';
import { PAGE_SIZE_OPTIONS, SERVER_PAGE_SIZE_OPTIONS } from './consts';
+function isServerPaginationUnsupported({
Review Comment:
Good catch. AG Grid had the same offset-producing path. I shared Table's capability check and made AG Grid's pagination controls fail closed when `ROW_OFFSET` is absent, while keeping the saved value and the explicit reset: [27709410](https://github.com/apache/superset/commit/277094100e3ea998de7f35837e1dc13332ac5cd4). The new AG Grid control test failed in six cases before the fix and passes now.
--
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] feat(semantic-layer): gate Table pagination on row offset support [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #44452:
URL: https://github.com/apache/superset/pull/44452#discussion_r4234100241
##########
superset-frontend/plugins/plugin-chart-table/test/controlPanel.test.tsx:
##########
@@ -473,6 +484,394 @@ test('columnOptions defaults type_generic to String when missing from datasource
);
});
+function getControl(name: string): ControlConfig {
+ for (const section of config.controlPanelSections) {
+ if (!section) continue;
+ for (const row of section.controlSetRows) {
+ for (const control of row) {
+ if (isCustomControlItem(control) && control.name === name) {
+ return control.config;
+ }
+ }
+ }
+ }
+ throw new Error(`Missing control: ${name}`);
+}
+
+const pagination = getControl('server_pagination');
+const pageLength = getControl('server_page_length');
+
+function panelState(
+ features?: string[],
+ type = 'semantic_view',
+ value = false,
+): ControlPanelState {
+ return {
+ datasource: {
+ id: 1,
+ uid: 'provider-view',
+ type,
+ semantic_view_features: features,
+ } as Dataset,
+ form_data: {
+ datasource: `1__${type}`,
+ viz_type: 'table',
+ server_pagination: value,
+ },
+ controls: { server_pagination: { type: 'CheckboxControl', value } },
+ slice: { slice_id: 1 },
+ common: {},
+ };
+}
+
+test('recomputes pagination controls when datasource metadata changes', () => {
+ const registry = getChartControlPanelRegistry();
+ registry.registerValue('table', {
+ controlPanelSections: [
+ {
+ label: 'Options',
+ expanded: true,
+ controlSetRows: [
+ [{ name: 'server_pagination', config: pagination }],
+ [{ name: 'server_page_length', config: pageLength }],
+ ],
+ },
+ ],
+ });
+ const state = panelState(['ROW_OFFSET']);
+ const props = {
+ actions: { setControlValue: jest.fn() },
+ chart: { chartStatus: 'success', queriesResponse: null },
+ controls: {
+ ...state.controls,
+ server_page_length: { type: 'SelectControl', value: 10 },
+ },
+ datasource_type: DatasourceType.SemanticView,
+ exploreState: state,
+ form_data: state.form_data,
+ isDatasourceMetaLoading: false,
+ errorMessage: null,
+ onQuery: jest.fn(),
+ onStop: jest.fn(),
+ canStopQuery: false,
+ chartIsStale: false,
+ } as unknown as ControlPanelsContainerProps;
+
+ try {
+ const { rerender } = render(<ControlPanelsContainer {...props} />, {
+ useRedux: true,
+ });
+ expect(screen.getByRole('checkbox')).toBeEnabled();
+ rerender(
+ <ControlPanelsContainer
+ {...props}
+ exploreState={{
+ ...props.exploreState,
+ datasource: panelState([]).datasource as Dataset,
+ }}
+ />,
+ );
+ expect(screen.getByRole('checkbox')).toBeDisabled();
+ } finally {
+ registry.remove('table');
+ }
+});
+
+test.each([
+ ['server_pagination', pagination],
+ ['server_page_length', pageLength],
+])('%s remaps when datasource capabilities change', (name, control) => {
+ const before = panelState(['ROW_OFFSET']);
+ const after = panelState([]);
+ const controlState = after.controls[name];
+
+ expect(control.shouldMapStateToProps?.(before, after, controlState)).toBe(
+ true,
+ );
+ expect(control.mapStateToProps?.(after, controlState)).toMatchObject({
+ disabled: true,
+ });
+});
+
+function renderPagination(state: ControlPanelState, onChange = jest.fn()) {
+ return render(
+ <CheckboxControl
+ name="server_pagination"
+ label="Server pagination"
+ {...pagination.mapStateToProps?.(state, state.controls.server_pagination)}
+ value={Boolean(state.controls.server_pagination.value)}
+ onChange={onChange}
+ />,
+ );
+}
+
+test.each([undefined, [], ['UNKNOWN'], ['row_offset']])(
+ 'disables unsupported semantic pagination with explanation: %s',
+ async features => {
+ const onChange = jest.fn();
+ const state = panelState(features);
+ renderPagination(state, onChange);
+ const checkbox = screen.getByRole('checkbox');
+ expect(pagination.type).toBe('CheckboxControl');
+ expect(checkbox).toBeDisabled();
+ expect(checkbox).toHaveAccessibleDescription(
+ 'This semantic view does not support server pagination.',
+ );
+ await userEvent.click(screen.getByText('Server pagination'));
+ expect(onChange).not.toHaveBeenCalled();
+ },
+);
+
+test('fails closed while semantic datasource metadata is unavailable', () => {
+ const state = { ...panelState(), datasource: null };
+ renderPagination(state);
+ expect(screen.getByRole('checkbox')).toBeDisabled();
+});
+
+test.each([
+ ['1__semantic_view', '2__semantic_view'],
+ ['cube__orders', 'cube__customers'],
+])(
+ 'withholds pagination advice for stale metadata %s -> %s',
+ (uid, selected) => {
+ const state = panelState([], 'semantic_view', true);
+ state.datasource = {
+ ...state.datasource,
+ id: 1,
+ uid,
+ type: 'semantic_view',
+ semantic_view_features: [],
+ } as Dataset;
+ state.form_data.datasource = selected;
+ const panel = getControl('server_pagination');
+ expect(
+ panel.mapStateToProps?.(state, state.controls.server_pagination),
+ ).toMatchObject({
+ disabled: true,
+ resetLabel: undefined,
+ disabledReason: undefined,
+ });
+ state.datasource = { ...state.datasource, id: 2, uid: selected } as Dataset;
+ expect(
+ panel.mapStateToProps?.(state, state.controls.server_pagination),
+ ).toMatchObject({
+ disabled: true,
+ resetLabel: 'Turn off server pagination',
+ disabledReason: 'This semantic view does not support server pagination.',
+ });
+ },
+);
+
+test.each([true, false])(
+ 'matches Explore id/type independently of provider uid: offset=%s',
+ supportsOffset => {
+ const state = panelState([], 'semantic_view', true);
+ state.datasource = {
+ ...state.datasource,
+ id: 42,
+ uid: 'provider-orders',
+ type: 'semantic_view',
+ semantic_view_features: supportsOffset ? ['ROW_OFFSET'] : [],
+ } as Dataset;
+ state.form_data.datasource = '42__semantic_view';
+ const panel = getControl('server_pagination');
+ expect(
+ panel.mapStateToProps?.(state, state.controls.server_pagination),
+ ).toMatchObject({
+ disabled: !supportsOffset,
+ resetLabel: 'Turn off server pagination',
+ disabledReason: 'This semantic view does not support server pagination.',
+ });
+ state.datasource = {
+ ...state.datasource,
+ id: 43,
+ uid: '42__semantic_view',
+ } as Dataset;
+ expect(
+ panel.mapStateToProps?.(state, state.controls.server_pagination),
+ ).toMatchObject({
+ disabled: true,
+ resetLabel: undefined,
+ disabledReason: undefined,
+ });
+ state.datasource = {
+ ...state.datasource,
+ id: 42,
+ type: 'table',
+ } as Dataset;
+ expect(
+ panel.mapStateToProps?.(state, state.controls.server_pagination),
+ ).toMatchObject({
+ disabled: true,
+ resetLabel: undefined,
+ disabledReason: undefined,
+ });
+ },
+);
+
+test.each([undefined, [], ['ROW_OFFSET']])(
+ 'matches opaque provider uid before parsing datasource type: %s',
+ features => {
+ const state = panelState(features);
+ state.datasource = { ...state.datasource, uid: 'cube__orders' } as Dataset;
+ state.form_data.datasource = 'cube__orders';
+ renderPagination(state);
+ const disabled = !features?.includes('ROW_OFFSET');
+ expect(screen.getByRole('checkbox').hasAttribute('disabled')).toBe(
+ disabled,
+ );
+ expect(
+ pageLength.mapStateToProps?.(state, state.controls.server_page_length),
+ ).toMatchObject({ disabled });
+ },
+);
+
+test.each([
+ ['table', 'semantic_view', true],
+ ['semantic_view', 'table', false],
+])(
+ 'trusts form datasource %s -> %s while metadata is stale',
+ (previous, next, disabled) => {
+ const state = panelState([], previous, true);
+ state.form_data.datasource = `2__${next}`;
+ renderPagination(state);
+ expect(screen.getByRole('checkbox').hasAttribute('disabled')).toBe(
+ disabled,
+ );
+ expect(
+ pageLength.mapStateToProps?.(state, state.controls.server_pagination),
+ ).toMatchObject({ disabled });
+ expect(state.controls.server_pagination.value).toBe(true);
+ },
+);
+
+test('ignores stale offset capability when switching opaque semantic UIDs', () => {
+ const state = panelState(['ROW_OFFSET'], 'semantic_view', true);
+ state.datasource = {
+ ...state.datasource,
+ uid: 'cube__orders',
+ type: 'semantic_view',
+ semantic_view_features: ['ROW_OFFSET'],
+ } as Dataset;
+ state.form_data.datasource = 'cube__customers';
+
+ for (const name of ['server_pagination', 'server_page_length']) {
+ expect(
+ getControl(name).mapStateToProps?.(state, state.controls[name]),
+ ).toMatchObject({ disabled: true });
+ }
+});
+
+test.each(['server_pagination', 'server_page_length'])(
+ '%s ignores stale offset capability from another semantic view',
+ name => {
+ const state = panelState(['ROW_OFFSET'], 'semantic_view', true);
+ state.datasource = {
+ ...state.datasource,
+ uid: '1__semantic_view',
+ } as Dataset;
+ state.form_data.datasource = '2__semantic_view';
+
+ expect(
+ getControl(name).mapStateToProps?.(state, state.controls[name]),
+ ).toMatchObject({ disabled: true });
+ expect(state.controls.server_pagination.value).toBe(true);
+ },
+);
+
+test('does not offer a reset before datasource metadata arrives', () => {
+ const state = {
+ ...panelState(undefined, 'semantic_view', true),
+ datasource: null,
+ };
+ renderPagination(state);
+ expect(
+ getControl('server_pagination').mapStateToProps?.(
+ state,
+ state.controls.server_pagination,
+ ),
+ ).toMatchObject({ disabledReason: undefined, resetLabel: undefined });
+ expect(screen.getByRole('checkbox')).toBeDisabled();
+ expect(screen.getByRole('checkbox')).toBeChecked();
+ expect(
+ screen.queryByRole('button', { name: 'Turn off server pagination' }),
+ ).not.toBeInTheDocument();
+});
+
+test.each([
+ ['semantic_view', ['ROW_OFFSET']],
+ ['table', undefined],
+ ['table', []],
+])('keeps %s pagination editable with %s', async (type, features) => {
+ const onChange = jest.fn();
+ renderPagination(panelState(features, type), onChange);
+ expect(screen.getByRole('checkbox')).toBeEnabled();
+ await userEvent.click(screen.getByRole('checkbox'));
+ expect(onChange).toHaveBeenCalledWith(true);
+ expect(
+ screen.queryByText(/does not support server pagination/),
+ ).not.toBeInTheDocument();
+});
+
+test('disables dependent page length without hiding or resetting saved state', () => {
+ const state = panelState([], 'semantic_view', true);
+ expect(
+ pageLength.mapStateToProps?.(state, state.controls.server_pagination),
+ ).toMatchObject({ disabled: true });
+ expect(
+ pageLength.visibility?.(
+ { ...state, actions: { setDatasource: jest.fn() }, exportState: {} },
+ {},
+ ),
+ ).toBe(true);
+ expect(state.controls.server_pagination.value).toBe(true);
+});
+
+test('saved true survives datasource changes until an explicit keyboard edit', async () => {
+ const saved = Object.freeze({ server_pagination: true });
+ const onEdit = jest.fn();
+ function Editor({ features }: { features?: string[] }) {
+ const [value, setValue] = useState<boolean>(saved.server_pagination);
+ const state = panelState(features, 'semantic_view', value);
+ return (
+ <CheckboxControl
+ name="server_pagination"
+ label="Server pagination"
+ {...pagination.mapStateToProps?.(
+ state,
+ state.controls.server_pagination,
+ )}
+ value={value}
+ onChange={next => {
+ setValue(next);
+ onEdit(next);
+ }}
+ />
+ );
+ }
+ const { rerender } = render(<Editor features={['ROW_OFFSET']} />);
+ expect(screen.getByRole('checkbox')).toBeChecked();
+ rerender(<Editor features={[]} />);
+ expect(screen.getByRole('checkbox')).toBeChecked();
+ expect(screen.getByRole('checkbox')).toBeDisabled();
+ expect(onEdit).not.toHaveBeenCalled();
+ await userEvent.tab();
+ expect(
+ screen.getByRole('button', { name: 'Turn off server pagination' }),
+ ).toHaveFocus();
+ await userEvent.type(
Review Comment:
`userEvent.type()` clicks its target before typing (unless `skipClick` is set), so here the click on "Turn off server pagination" already calls `onChange(false)` and removes the button before `{enter}` is sent. The unchecked-state and `toHaveBeenCalledTimes(1)` assertions then pass without any keyboard activation, so a regression that breaks Enter on the reset button while leaving mouse clicks working would still go green, and this is the only test that claims to cover the keyboard reset. Could this use `await userEvent.keyboard('{Enter}')` after the existing focus assertion so the keyboard path is actually exercised?
--
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] feat(semantic-layer): gate Table pagination on row offset support [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on PR #44452:
URL: https://github.com/apache/superset/pull/44452#issuecomment-6133715257
@mikebridge I re-reviewed the post-approval delta at 26ce647755ab89accc5caf052d9108cfe381385b and it looks correct: id/type metadata matching, stale-metadata gating (including @geido's other-semantic-view case), and per-instance checkbox ids. The branch now conflicts with master in UPDATING.md. Could you merge master? I'll merge once it's green.
--
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] feat(semantic-layer): gate Table pagination on row offset support [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #44452:
URL: https://github.com/apache/superset/pull/44452#issuecomment-5943453551
The AG Grid follow-up and the earlier Bito fixes look good.
Bito's duplicated `mapStateToProps` thread is still open. Sharing that helper would be nice, but I'm fine with a quick reply noting the deferral if you'd rather keep this PR tight.
Otherwise happy to approve.
--
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] feat(semantic-layer): gate Table pagination on row offset support [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44452:
URL: https://github.com/apache/superset/pull/44452#issuecomment-5745932883
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(semantic-layer): gate Table pagination on row offset support [superset]
Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
mikebridge commented on PR #44452:
URL: https://github.com/apache/superset/pull/44452#issuecomment-6042820389
@aminghadersohi @rebenitez1802 thanks for the earlier approvals. Amin's follow-up points are addressed: the label double-toggle fix is in 7fabdec758505f130a814f79c5979dd83f43a383 (with a regression test), and the earlier threads are fixed in 4ff18ddf7ba5370e645be1630150df04a97fcf49 (replies are inline). Since the branch changed after your reviews, would you mind taking another look when you have a moment? Thanks!
--
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] feat(semantic-layer): gate Table pagination on row offset support [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #44452:
URL: https://github.com/apache/superset/pull/44452#discussion_r4227043698
##########
superset-frontend/src/explore/components/controls/CheckboxControl.tsx:
##########
@@ -50,24 +58,77 @@ const CheckBoxControlWrapper = styled.div`
export default function CheckboxControl({
value = false,
label,
+ disabled = false,
+ disabledReason,
+ resetLabel,
onChange = () => {},
...restProps
}: CheckboxControlProps): JSX.Element {
+ const explanationId = useId();
+ const generatedCheckboxId = useId();
+ const checkboxId = restProps.name || generatedCheckboxId;
Review Comment:
Every `CheckboxControl` that passes a `name` now renders its input with `id={name}` (line 69/78), but column-config popovers reuse the same key (e.g. `showCellBars`) for each column and keep their DOM after closing. With two columns' popovers opened in turn, both inputs get `id="showCellBars"`, so column B's label `htmlFor` and the `document.getElementById(checkboxId)?.focus()` lookup at line 117 resolve to column A's input, and a click in the label area can toggle A's setting instead of B's. Should the input/label id be an instance-unique value (e.g. the generated `useId()` value, optionally prefixed with the name) rather than the shared control name?
--
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] feat(semantic-layer): gate Table pagination on row offset support [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi merged PR #44452:
URL: https://github.com/apache/superset/pull/44452
--
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] feat(semantic-layer): gate Table pagination on row offset support [superset]
Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
mikebridge commented on PR #44452:
URL: https://github.com/apache/superset/pull/44452#issuecomment-6152253745
@aminghadersohi Merged master in [b7c67ffc58](https://github.com/apache/superset/commit/b7c67ffc589ea0ff659dccc4f2650597df8c6c50). Per the approved UPDATING audit, I moved the Explore header/accessibility note verbatim into the PR's BEFORE/AFTER section because it describes a UI improvement rather than an upgrade action. The titled pagination/ROW_OFFSET compatibility entry stays unchanged in UPDATING.md; all other prose is preserved. Conflicts were mechanical unions. Verification: 1,210 backend/SDK tests, 293 frontend tests and branch-file hooks pass. Authored and verified by Codex.
--
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] feat(semantic-layer): gate Table pagination on row offset support [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #44452:
URL: https://github.com/apache/superset/pull/44452#discussion_r4211905512
##########
superset-frontend/packages/superset-ui-chart-controls/src/utils/isServerPaginationUnsupported.ts:
##########
@@ -0,0 +1,40 @@
+/**
+ * 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 { DatasourceType } from '@superset-ui/core';
+import type { ControlPanelState } from '../types';
+
+/** SemanticViewFeature value for providers that support row offsets. */
+export const ROW_OFFSET = 'ROW_OFFSET';
+
+/** Keep semantic pagination disabled until the view declares offset support. */
+export function isServerPaginationUnsupported({
+ datasource,
+ form_data,
+}: ControlPanelState): boolean {
+ const datasourceType =
+ form_data.datasource?.split('__')[1] ?? datasource?.type;
Review Comment:
After an in-Explore switch `form_data.datasource` holds the provider `uid`; one containing `__` (e.g. `cube__orders`) parses as a non-semantic type, so a view without ROW_OFFSET gets pagination enabled. Trust `datasource.type` when its uid matches the form.
```suggestion
const datasourceType =
datasource && 'uid' in datasource && datasource.uid === form_data.datasource
? datasource.type
: (form_data.datasource?.split('__')[1] ?? datasource?.type);
```
--
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] feat(semantic-layer): gate Table pagination on row offset support [superset]
Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
mikebridge commented on PR #44452:
URL: https://github.com/apache/superset/pull/44452#issuecomment-6083994263
@geido Follow-up: the label/reason item I flagged in my previous comment is fixed in `a6c46eac4f`.
`hasMatchingDatasourceMetadata` is now the single predicate behind all three decisions — the capability check, `disabledReason` and `resetLabel` — in both the Table and AG Grid control panels. So while metadata is pending or belongs to a previously selected view, the control is disabled with no explanation and no reset offer, instead of describing the selected view using another view's metadata. Once the UID matches the selection, both strings appear as before.
The parametrized `withholds pagination advice for stale metadata …` tests pin the whole transition for both canonical (`1__semantic_view` → `2__semantic_view`) and opaque (`cube__orders` → `cube__customers`) UIDs, and the two pending-metadata tests now assert the neutral reason as well. Six tests fail without the change; both control-panel suites are 65/65 with 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] feat(semantic-layer): gate Table pagination on row offset support [superset]
Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
mikebridge commented on PR #44452:
URL: https://github.com/apache/superset/pull/44452#issuecomment-5956548372
@rusackas thanks for taking another look. I have replied on the Bito thread: I would like to keep this PR tight, so sharing the `mapStateToProps` blocks between the two table plugins is deferred to a follow-up (sc-124090). Nothing else is open on my side.
--
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] feat(semantic-layer): gate Table pagination on row offset support [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #44452:
URL: https://github.com/apache/superset/pull/44452#discussion_r4232582212
##########
superset-frontend/packages/superset-ui-chart-controls/src/utils/isServerPaginationUnsupported.ts:
##########
@@ -0,0 +1,62 @@
+/**
+ * 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 { DatasourceType } from '@superset-ui/core';
+import type { ControlPanelState } from '../types';
+
+/** SemanticViewFeature value for providers that support row offsets. */
+export const ROW_OFFSET = 'ROW_OFFSET';
+
+/** Only selected-datasource metadata can describe its capabilities. */
+export function hasMatchingDatasourceMetadata({
+ datasource,
+ form_data,
+}: Pick<ControlPanelState, 'datasource' | 'form_data'>): boolean {
+ return Boolean(
+ datasource &&
+ 'uid' in datasource &&
+ datasource.uid === form_data.datasource,
Review Comment:
On a normal Explore load `form_data.datasource` is `<id>__semantic_view` (set in `ExploreGetCommand`), while `datasource.uid` for a semantic view is the provider's opaque `implementation.uid()`, so this equality is false. `hasMatchingDatasourceMetadata` then returns false, `features` is dropped to `undefined`, and `isServerPaginationUnsupported` returns true even for a view that advertises `ROW_OFFSET`. A user opening a saved or fresh chart on such a view would see Server pagination disabled and never able to enable it, unless the provider's uid happens to be literally `<id>__semantic_view`. Should the match compare `datasource.id`/`type` against the parsed form value, or is there a path that rewrites `form_data.datasource` to the provider uid before the control panel runs?
--
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] feat(semantic-layer): gate Table pagination on row offset support [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #44452:
URL: https://github.com/apache/superset/pull/44452#discussion_r4236260013
##########
superset-frontend/src/explore/components/controls/CheckboxControl.tsx:
##########
@@ -50,24 +58,77 @@ const CheckBoxControlWrapper = styled.div`
export default function CheckboxControl({
value = false,
label,
+ disabled = false,
+ disabledReason,
+ resetLabel,
onChange = () => {},
...restProps
}: CheckboxControlProps): JSX.Element {
+ const explanationId = useId();
+ const generatedCheckboxId = useId();
+ const checkboxId = restProps.name || generatedCheckboxId;
Review Comment:
Now that the checkbox input gets `id={name}`, controls that reuse a static `name` across several mounted instances end up with duplicate DOM ids. The annotation layer editors do this (`annotation-layer-show-markers`, `annotation-layer-hide-line`, `annotation-override-time_range`, ...), and `ControlPopover` leaves closed popovers mounted (`destroyOnHidden` defaults to false). After opening layer A and then layer B, B's header label `htmlFor` resolves to A's hidden input, and the new `document.getElementById(checkboxId)?.focus()` on label click also lands on A's input, so B's checkbox ends up with the wrong accessible-name association and focus goes to a hidden element. Should the input id be scoped per instance (for example by always using the generated id for the input and only forwarding `name` for the `data-test` attributes)?
--
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] feat(semantic-layer): gate Table pagination on row offset support [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #44452:
URL: https://github.com/apache/superset/pull/44452#discussion_r4202506952
##########
superset-frontend/src/explore/components/controls/CheckboxControl.tsx:
##########
@@ -50,24 +53,58 @@ const CheckBoxControlWrapper = styled.div`
export default function CheckboxControl({
value = false,
label,
+ disabled = false,
+ disabledReason,
+ resetLabel,
onChange = () => {},
...restProps
}: CheckboxControlProps): JSX.Element {
+ const explanationId = useId();
+ const generatedCheckboxId = useId();
+ const checkboxId = restProps.name || generatedCheckboxId;
const handleChange = useCallback((): void => {
- onChange(!value);
- }, [onChange, value]);
+ if (!disabled) {
+ onChange(!value);
+ }
+ }, [disabled, onChange, value]);
- const checkbox = <Checkbox onChange={handleChange} checked={!!value} />;
+ const checkbox = (
+ <Checkbox
+ id={checkboxId}
+ onChange={handleChange}
+ checked={!!value}
+ disabled={disabled}
+ aria-describedby={disabled && disabledReason ? explanationId : undefined}
+ />
+ );
+ const explanation = disabled && disabledReason && (
+ <p id={explanationId}>{disabledReason}</p>
+ );
if (label) {
return (
<CheckBoxControlWrapper>
<ControlHeader
{...restProps}
+ name={checkboxId}
label={label}
leftNode={checkbox}
- onClick={handleChange}
+ onClick={disabled ? undefined : handleChange}
Review Comment:
Clicking the label text now toggles twice: the span's `onClick` runs, then the outer `<label htmlFor>` re-dispatches the click to the input. With real state the box flips on and straight back off, which is why your exactly-once test fails in `sharded-jest-tests (8)`.
```suggestion
onClick={
disabled
? undefined
: (event?: { preventDefault: () => void }) => {
// Stop the associated label from toggling the input again.
event?.preventDefault();
handleChange();
}
}
```
--
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] feat(semantic-layer): gate Table pagination on row offset support [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44452:
URL: https://github.com/apache/superset/pull/44452#issuecomment-5756008905
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 an incremental AI Review.
- `/review full` - 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] feat(semantic-layer): gate Table pagination on row offset support [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44452:
URL: https://github.com/apache/superset/pull/44452#issuecomment-5841636069
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 an incremental AI Review.
- `/review full` - 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] feat(semantic-layer): gate Table pagination on row offset support [superset]
Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
mikebridge commented on code in PR #44452:
URL: https://github.com/apache/superset/pull/44452#discussion_r4233943999
##########
superset-frontend/packages/superset-ui-chart-controls/src/utils/isServerPaginationUnsupported.ts:
##########
@@ -0,0 +1,62 @@
+/**
+ * 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 { DatasourceType } from '@superset-ui/core';
+import type { ControlPanelState } from '../types';
+
+/** SemanticViewFeature value for providers that support row offsets. */
+export const ROW_OFFSET = 'ROW_OFFSET';
+
+/** Only selected-datasource metadata can describe its capabilities. */
+export function hasMatchingDatasourceMetadata({
+ datasource,
+ form_data,
+}: Pick<ControlPanelState, 'datasource' | 'form_data'>): boolean {
+ return Boolean(
+ datasource &&
+ 'uid' in datasource &&
+ datasource.uid === form_data.datasource,
Review Comment:
You're right, and thank you — this was a real regression, not just a theoretical one.
I confirmed your reading end to end: `form_data.datasource` is built as `f"{self._datasource_id}__{self._datasource_type}"` in `ExploreGetCommand` (`superset/commands/explore/get.py:137`), while `SemanticView.uid` returns `self.implementation.uid()` — the provider's opaque id — and the payload carries `{"id": self.id, "uid": self.uid, "type": "semantic_view"}` (`superset/semantic_layers/models.py:524-525`, `:611-613`). So the equality was false for the *correct* datasource, features dropped to `undefined`, and a view advertising `ROW_OFFSET` had Server pagination disabled with no way to enable it. Datasets were unaffected only because `BaseDatasource.uid` genuinely is `f"{self.id}__{self.type}"`.
To answer your question directly: nothing rewrites `form_data.datasource` to the provider uid before the control panel runs, so comparing against `datasource.id`/`type` is the right fix. `13bf5c46cd` parses `<id>__<type>` from the form value and compares it to the metadata's `id` and `type`, keeping uid equality only as the fallback for a selection that isn't of that canonical shape — so opaque provider uids still work for stale detection without being required to match the host's selection key.
Worth flagging why this slipped through review: the existing fixtures used `uid: '1__semantic_view'`, a shape that satisfied the predicate under test, so the suite couldn't contradict it. The fixtures now use the real Explore shape, and with that correction the *pre-existing* `keeps semantic_view pagination editable with ['ROW_OFFSET']` test fails against the old helper — 16 tests fail in total, across both plugins, so the old predicate was wrong in several directions and not only the one you reported. All 69 pass with the fix. The new test also pins the stale case with `uid` deliberately set to `'42__semantic_view'`, so reinstating uid equality would break 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] feat(semantic-layer): gate Table pagination on row offset support [superset]
Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
mikebridge commented on code in PR #44452:
URL: https://github.com/apache/superset/pull/44452#discussion_r4249790419
##########
superset-frontend/src/explore/components/controls/CheckboxControl.tsx:
##########
@@ -50,24 +58,77 @@ const CheckBoxControlWrapper = styled.div`
export default function CheckboxControl({
value = false,
label,
+ disabled = false,
+ disabledReason,
+ resetLabel,
onChange = () => {},
...restProps
}: CheckboxControlProps): JSX.Element {
+ const explanationId = useId();
+ const generatedCheckboxId = useId();
+ const checkboxId = restProps.name || generatedCheckboxId;
Review Comment:
Confirmed: two retained CheckboxControl instances with the same name shared an input ID. Each input now uses useId(), and ControlHeader receives that ID separately from the control name so the label association and focus lookup target the correct instance while existing header names stay stable.
Added a regression with two showCellBars controls: it fails before the fix and passes with separate accessible labels, distinct IDs, focus on the second checkbox, and only its change handler called. All 268 focused frontend tests and branch hooks pass. Implemented and independently reviewed with Codex and Claude.
--
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] feat(semantic-layer): gate Table pagination on row offset support [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #44452:
URL: https://github.com/apache/superset/pull/44452#discussion_r4260248619
##########
superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx:
##########
@@ -438,6 +440,16 @@ const config: ControlPanelConfig = {
'Enable server side pagination of results (experimental feature)',
),
default: false,
+ shouldMapStateToProps: () => true,
+ mapStateToProps: state => ({
+ disabled: isServerPaginationUnsupported(state),
+ disabledReason: hasMatchingDatasourceMetadata(state)
+ ? t('This semantic view does not support server pagination.')
+ : undefined,
+ resetLabel: hasMatchingDatasourceMetadata(state)
Review Comment:
With Server pagination now disabled for a view without `ROW_OFFSET`, the `row_limit` validator (`validateServerPagination`) still fails any limit above `SQL_MAX_ROW` with "Server pagination needs to be enabled for values over …", and the row-limit choices still offer values up to `TABLE_VIZ_MAX_ROW_SERVER`. A saved chart with pagination on and a limit like 200000 hits this right after the author uses "Turn off server pagination": the control then reports an error whose only suggested fix is the checkbox that is now disabled. Should the row-limit validation/choices account for the unsupported case, or the message point at lowering the limit instead? The same applies to the AG Grid panel.
--
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] feat(semantic-layer): gate Table pagination on row offset support [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44452:
URL: https://github.com/apache/superset/pull/44452#issuecomment-5919495138
<!-- Bito Reply -->
The implementation correctly addresses the issue by introducing `isServerPaginationUnsupported` to gate the `server_pagination` control based on the `ROW_OFFSET` feature flag. This helper function checks if the datasource is a semantic view and verifies the presence of `ROW_OFFSET` in `semantic_view_features`, ensuring that the control is disabled with an appropriate explanation when the capability is missing. This approach effectively prevents the provider error by disabling the UI control for unsupported semantic views, while allowing existing SQL datasets to maintain their current behavior.
**superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx**
```
function isServerPaginationUnsupported({
datasource,
form_data,
}: ControlPanelState): boolean {
const isSemanticView =
datasource?.type === 'semantic_view' ||
(!datasource && form_data.datasource?.endsWith('__semantic_view'));
const features =
datasource && 'semantic_view_features' in datasource
? datasource.semantic_view_features
: undefined;
return Boolean(isSemanticView && !features?.includes('ROW_OFFSET'));
}
```
--
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] feat(semantic-layer): gate Table pagination on row offset support [superset]
Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
mikebridge commented on code in PR #44452:
URL: https://github.com/apache/superset/pull/44452#discussion_r4209084340
##########
superset-frontend/src/explore/components/controls/CheckboxControl.tsx:
##########
@@ -50,14 +53,30 @@ const CheckBoxControlWrapper = styled.div`
export default function CheckboxControl({
value = false,
label,
+ disabled = false,
+ disabledReason,
+ resetLabel,
onChange = () => {},
...restProps
}: CheckboxControlProps): JSX.Element {
+ const explanationId = useId();
const handleChange = useCallback((): void => {
- onChange(!value);
- }, [onChange, value]);
+ if (!disabled) {
+ onChange(!value);
+ }
+ }, [disabled, onChange, value]);
- const checkbox = <Checkbox onChange={handleChange} checked={!!value} />;
+ const checkbox = (
+ <Checkbox
+ onChange={handleChange}
+ checked={!!value}
+ disabled={disabled}
+ aria-describedby={disabled && disabledReason ? explanationId : undefined}
+ />
Review Comment:
Good catch. `aria-describedby` explained the disabled state but did not name the checkbox, and the outer label had no matching input ID.
Could we link the existing visible label directly? Commit 4ff18ddf gives the input an ID and passes it to `ControlHeader`; the accessible-name and description tests pass for both string and ReactNode labels.
##########
superset-frontend/src/explore/components/controls/CheckboxControl.test.tsx:
##########
@@ -49,4 +49,37 @@ describe('CheckboxControl', () => {
await userEvent.click(label);
expect(defaultProps.onChange).toHaveBeenCalled();
});
+
+ test('explains a disabled checkbox without a label', () => {
+ render(
+ setup({
+ label: undefined,
+ disabled: true,
+ disabledReason: 'Unavailable',
+ }),
+ );
+
+ const checkbox = screen.getByRole('checkbox');
+ expect(checkbox).toHaveAccessibleDescription('Unavailable');
+ expect(screen.getByText('Unavailable')).toBeInTheDocument();
+ });
+
+ test('retains the control header and description when disabled', () => {
+ const { container } = render(
+ setup({
+ disabled: true,
+ description: 'Why this control is disabled',
+ hovered: true,
+ renderTrigger: true,
+ }),
+ );
+
+ expect(
+ container.querySelector('[data-test="show_legend-header"]'),
+ ).toBeInTheDocument();
+ expect(
+ container.querySelector('[data-test="show_legend-description-icon"]'),
+ ).toBeInTheDocument();
+ expect(screen.getByRole('checkbox')).toBeDisabled();
+ });
Review Comment:
Good catch on the missing regression pin. Could we keep the defensive `handleChange` guard as well as the disabled label click restriction?
Commit 4ff18ddf adds a disabled-label test that fails when both guards are removed, and the enabled-label test now asserts exactly one callback so the label association does not double-toggle.
##########
superset-frontend/src/explore/components/controls/CheckboxControl.tsx:
##########
@@ -50,24 +53,58 @@ const CheckBoxControlWrapper = styled.div`
export default function CheckboxControl({
value = false,
label,
+ disabled = false,
+ disabledReason,
+ resetLabel,
onChange = () => {},
...restProps
}: CheckboxControlProps): JSX.Element {
+ const explanationId = useId();
+ const generatedCheckboxId = useId();
+ const checkboxId = restProps.name || generatedCheckboxId;
const handleChange = useCallback((): void => {
- onChange(!value);
- }, [onChange, value]);
+ if (!disabled) {
+ onChange(!value);
+ }
+ }, [disabled, onChange, value]);
- const checkbox = <Checkbox onChange={handleChange} checked={!!value} />;
+ const checkbox = (
+ <Checkbox
+ id={checkboxId}
+ onChange={handleChange}
+ checked={!!value}
+ disabled={disabled}
+ aria-describedby={disabled && disabledReason ? explanationId : undefined}
+ />
+ );
+ const explanation = disabled && disabledReason && (
+ <p id={explanationId}>{disabledReason}</p>
+ );
if (label) {
return (
<CheckBoxControlWrapper>
<ControlHeader
{...restProps}
+ name={checkboxId}
label={label}
leftNode={checkbox}
- onClick={handleChange}
+ onClick={disabled ? undefined : handleChange}
Review Comment:
Good catch. Could you take another look at the [follow-up commit](https://github.com/apache/superset/commit/7fabdec758505f130a814f79c5979dd83f43a383)?
The label-text click prevents the second native activation and explicitly focuses the checkbox, preserving the label behavior. The new click and focus regressions failed against the broken variants and pass with the fix; the new-head CI is running.
--
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] feat(semantic-layer): gate Table pagination on row offset support [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #44452:
URL: https://github.com/apache/superset/pull/44452#discussion_r4201535377
##########
superset-frontend/src/explore/components/controls/CheckboxControl.tsx:
##########
@@ -50,14 +53,30 @@ const CheckBoxControlWrapper = styled.div`
export default function CheckboxControl({
value = false,
label,
+ disabled = false,
+ disabledReason,
+ resetLabel,
onChange = () => {},
...restProps
}: CheckboxControlProps): JSX.Element {
+ const explanationId = useId();
const handleChange = useCallback((): void => {
- onChange(!value);
- }, [onChange, value]);
+ if (!disabled) {
+ onChange(!value);
+ }
+ }, [disabled, onChange, value]);
- const checkbox = <Checkbox onChange={handleChange} checked={!!value} />;
+ const checkbox = (
+ <Checkbox
+ onChange={handleChange}
+ checked={!!value}
+ disabled={disabled}
+ aria-describedby={disabled && disabledReason ? explanationId : undefined}
+ />
Review Comment:
Moving the label out of `<Checkbox>` leaves the disabled checkbox with no accessible name (it had one at 277094100e), so a screen reader announces an unnamed dimmed checkbox plus the reason; the outer `<label htmlFor={name}>` matches no id.
```suggestion
aria-describedby={disabled && disabledReason ? explanationId : undefined}
aria-label={typeof label === 'string' ? label : undefined}
/>
```
##########
superset-frontend/src/explore/components/controls/CheckboxControl.test.tsx:
##########
@@ -49,4 +49,37 @@ describe('CheckboxControl', () => {
await userEvent.click(label);
expect(defaultProps.onChange).toHaveBeenCalled();
});
+
+ test('explains a disabled checkbox without a label', () => {
+ render(
+ setup({
+ label: undefined,
+ disabled: true,
+ disabledReason: 'Unavailable',
+ }),
+ );
+
+ const checkbox = screen.getByRole('checkbox');
+ expect(checkbox).toHaveAccessibleDescription('Unavailable');
+ expect(screen.getByText('Unavailable')).toBeInTheDocument();
+ });
+
+ test('retains the control header and description when disabled', () => {
+ const { container } = render(
+ setup({
+ disabled: true,
+ description: 'Why this control is disabled',
+ hovered: true,
+ renderTrigger: true,
+ }),
+ );
+
+ expect(
+ container.querySelector('[data-test="show_legend-header"]'),
+ ).toBeInTheDocument();
+ expect(
+ container.querySelector('[data-test="show_legend-description-icon"]'),
+ ).toBeInTheDocument();
+ expect(screen.getByRole('checkbox')).toBeDisabled();
+ });
Review Comment:
Disabled label clicks are blocked twice (`onClick` undefined and `if (!disabled)`); removing both still passes every test, so nothing pins it.
```suggestion
});
test('ignores label clicks while disabled', async () => {
const onChange = jest.fn();
render(setup({ disabled: true, onChange }));
await userEvent.click(screen.getByText('checkbox label'));
expect(onChange).not.toHaveBeenCalled();
});
```
--
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] feat(semantic-layer): gate Table pagination on row offset support [superset]
Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
mikebridge commented on code in PR #44452:
URL: https://github.com/apache/superset/pull/44452#discussion_r4235503329
##########
superset-frontend/plugins/plugin-chart-table/test/controlPanel.test.tsx:
##########
@@ -473,6 +484,394 @@ test('columnOptions defaults type_generic to String when missing from datasource
);
});
+function getControl(name: string): ControlConfig {
+ for (const section of config.controlPanelSections) {
+ if (!section) continue;
+ for (const row of section.controlSetRows) {
+ for (const control of row) {
+ if (isCustomControlItem(control) && control.name === name) {
+ return control.config;
+ }
+ }
+ }
+ }
+ throw new Error(`Missing control: ${name}`);
+}
+
+const pagination = getControl('server_pagination');
+const pageLength = getControl('server_page_length');
+
+function panelState(
+ features?: string[],
+ type = 'semantic_view',
+ value = false,
+): ControlPanelState {
+ return {
+ datasource: {
+ id: 1,
+ uid: 'provider-view',
+ type,
+ semantic_view_features: features,
+ } as Dataset,
+ form_data: {
+ datasource: `1__${type}`,
+ viz_type: 'table',
+ server_pagination: value,
+ },
+ controls: { server_pagination: { type: 'CheckboxControl', value } },
+ slice: { slice_id: 1 },
+ common: {},
+ };
+}
+
+test('recomputes pagination controls when datasource metadata changes', () => {
+ const registry = getChartControlPanelRegistry();
+ registry.registerValue('table', {
+ controlPanelSections: [
+ {
+ label: 'Options',
+ expanded: true,
+ controlSetRows: [
+ [{ name: 'server_pagination', config: pagination }],
+ [{ name: 'server_page_length', config: pageLength }],
+ ],
+ },
+ ],
+ });
+ const state = panelState(['ROW_OFFSET']);
+ const props = {
+ actions: { setControlValue: jest.fn() },
+ chart: { chartStatus: 'success', queriesResponse: null },
+ controls: {
+ ...state.controls,
+ server_page_length: { type: 'SelectControl', value: 10 },
+ },
+ datasource_type: DatasourceType.SemanticView,
+ exploreState: state,
+ form_data: state.form_data,
+ isDatasourceMetaLoading: false,
+ errorMessage: null,
+ onQuery: jest.fn(),
+ onStop: jest.fn(),
+ canStopQuery: false,
+ chartIsStale: false,
+ } as unknown as ControlPanelsContainerProps;
+
+ try {
+ const { rerender } = render(<ControlPanelsContainer {...props} />, {
+ useRedux: true,
+ });
+ expect(screen.getByRole('checkbox')).toBeEnabled();
+ rerender(
+ <ControlPanelsContainer
+ {...props}
+ exploreState={{
+ ...props.exploreState,
+ datasource: panelState([]).datasource as Dataset,
+ }}
+ />,
+ );
+ expect(screen.getByRole('checkbox')).toBeDisabled();
+ } finally {
+ registry.remove('table');
+ }
+});
+
+test.each([
+ ['server_pagination', pagination],
+ ['server_page_length', pageLength],
+])('%s remaps when datasource capabilities change', (name, control) => {
+ const before = panelState(['ROW_OFFSET']);
+ const after = panelState([]);
+ const controlState = after.controls[name];
+
+ expect(control.shouldMapStateToProps?.(before, after, controlState)).toBe(
+ true,
+ );
+ expect(control.mapStateToProps?.(after, controlState)).toMatchObject({
+ disabled: true,
+ });
+});
+
+function renderPagination(state: ControlPanelState, onChange = jest.fn()) {
+ return render(
+ <CheckboxControl
+ name="server_pagination"
+ label="Server pagination"
+ {...pagination.mapStateToProps?.(state, state.controls.server_pagination)}
+ value={Boolean(state.controls.server_pagination.value)}
+ onChange={onChange}
+ />,
+ );
+}
+
+test.each([undefined, [], ['UNKNOWN'], ['row_offset']])(
+ 'disables unsupported semantic pagination with explanation: %s',
+ async features => {
+ const onChange = jest.fn();
+ const state = panelState(features);
+ renderPagination(state, onChange);
+ const checkbox = screen.getByRole('checkbox');
+ expect(pagination.type).toBe('CheckboxControl');
+ expect(checkbox).toBeDisabled();
+ expect(checkbox).toHaveAccessibleDescription(
+ 'This semantic view does not support server pagination.',
+ );
+ await userEvent.click(screen.getByText('Server pagination'));
+ expect(onChange).not.toHaveBeenCalled();
+ },
+);
+
+test('fails closed while semantic datasource metadata is unavailable', () => {
+ const state = { ...panelState(), datasource: null };
+ renderPagination(state);
+ expect(screen.getByRole('checkbox')).toBeDisabled();
+});
+
+test.each([
+ ['1__semantic_view', '2__semantic_view'],
+ ['cube__orders', 'cube__customers'],
+])(
+ 'withholds pagination advice for stale metadata %s -> %s',
+ (uid, selected) => {
+ const state = panelState([], 'semantic_view', true);
+ state.datasource = {
+ ...state.datasource,
+ id: 1,
+ uid,
+ type: 'semantic_view',
+ semantic_view_features: [],
+ } as Dataset;
+ state.form_data.datasource = selected;
+ const panel = getControl('server_pagination');
+ expect(
+ panel.mapStateToProps?.(state, state.controls.server_pagination),
+ ).toMatchObject({
+ disabled: true,
+ resetLabel: undefined,
+ disabledReason: undefined,
+ });
+ state.datasource = { ...state.datasource, id: 2, uid: selected } as Dataset;
+ expect(
+ panel.mapStateToProps?.(state, state.controls.server_pagination),
+ ).toMatchObject({
+ disabled: true,
+ resetLabel: 'Turn off server pagination',
+ disabledReason: 'This semantic view does not support server pagination.',
+ });
+ },
+);
+
+test.each([true, false])(
+ 'matches Explore id/type independently of provider uid: offset=%s',
+ supportsOffset => {
+ const state = panelState([], 'semantic_view', true);
+ state.datasource = {
+ ...state.datasource,
+ id: 42,
+ uid: 'provider-orders',
+ type: 'semantic_view',
+ semantic_view_features: supportsOffset ? ['ROW_OFFSET'] : [],
+ } as Dataset;
+ state.form_data.datasource = '42__semantic_view';
+ const panel = getControl('server_pagination');
+ expect(
+ panel.mapStateToProps?.(state, state.controls.server_pagination),
+ ).toMatchObject({
+ disabled: !supportsOffset,
+ resetLabel: 'Turn off server pagination',
+ disabledReason: 'This semantic view does not support server pagination.',
+ });
+ state.datasource = {
+ ...state.datasource,
+ id: 43,
+ uid: '42__semantic_view',
+ } as Dataset;
+ expect(
+ panel.mapStateToProps?.(state, state.controls.server_pagination),
+ ).toMatchObject({
+ disabled: true,
+ resetLabel: undefined,
+ disabledReason: undefined,
+ });
+ state.datasource = {
+ ...state.datasource,
+ id: 42,
+ type: 'table',
+ } as Dataset;
+ expect(
+ panel.mapStateToProps?.(state, state.controls.server_pagination),
+ ).toMatchObject({
+ disabled: true,
+ resetLabel: undefined,
+ disabledReason: undefined,
+ });
+ },
+);
+
+test.each([undefined, [], ['ROW_OFFSET']])(
+ 'matches opaque provider uid before parsing datasource type: %s',
+ features => {
+ const state = panelState(features);
+ state.datasource = { ...state.datasource, uid: 'cube__orders' } as Dataset;
+ state.form_data.datasource = 'cube__orders';
+ renderPagination(state);
+ const disabled = !features?.includes('ROW_OFFSET');
+ expect(screen.getByRole('checkbox').hasAttribute('disabled')).toBe(
+ disabled,
+ );
+ expect(
+ pageLength.mapStateToProps?.(state, state.controls.server_page_length),
+ ).toMatchObject({ disabled });
+ },
+);
+
+test.each([
+ ['table', 'semantic_view', true],
+ ['semantic_view', 'table', false],
+])(
+ 'trusts form datasource %s -> %s while metadata is stale',
+ (previous, next, disabled) => {
+ const state = panelState([], previous, true);
+ state.form_data.datasource = `2__${next}`;
+ renderPagination(state);
+ expect(screen.getByRole('checkbox').hasAttribute('disabled')).toBe(
+ disabled,
+ );
+ expect(
+ pageLength.mapStateToProps?.(state, state.controls.server_pagination),
+ ).toMatchObject({ disabled });
+ expect(state.controls.server_pagination.value).toBe(true);
+ },
+);
+
+test('ignores stale offset capability when switching opaque semantic UIDs', () => {
+ const state = panelState(['ROW_OFFSET'], 'semantic_view', true);
+ state.datasource = {
+ ...state.datasource,
+ uid: 'cube__orders',
+ type: 'semantic_view',
+ semantic_view_features: ['ROW_OFFSET'],
+ } as Dataset;
+ state.form_data.datasource = 'cube__customers';
+
+ for (const name of ['server_pagination', 'server_page_length']) {
+ expect(
+ getControl(name).mapStateToProps?.(state, state.controls[name]),
+ ).toMatchObject({ disabled: true });
+ }
+});
+
+test.each(['server_pagination', 'server_page_length'])(
+ '%s ignores stale offset capability from another semantic view',
+ name => {
+ const state = panelState(['ROW_OFFSET'], 'semantic_view', true);
+ state.datasource = {
+ ...state.datasource,
+ uid: '1__semantic_view',
+ } as Dataset;
+ state.form_data.datasource = '2__semantic_view';
+
+ expect(
+ getControl(name).mapStateToProps?.(state, state.controls[name]),
+ ).toMatchObject({ disabled: true });
+ expect(state.controls.server_pagination.value).toBe(true);
+ },
+);
+
+test('does not offer a reset before datasource metadata arrives', () => {
+ const state = {
+ ...panelState(undefined, 'semantic_view', true),
+ datasource: null,
+ };
+ renderPagination(state);
+ expect(
+ getControl('server_pagination').mapStateToProps?.(
+ state,
+ state.controls.server_pagination,
+ ),
+ ).toMatchObject({ disabledReason: undefined, resetLabel: undefined });
+ expect(screen.getByRole('checkbox')).toBeDisabled();
+ expect(screen.getByRole('checkbox')).toBeChecked();
+ expect(
+ screen.queryByRole('button', { name: 'Turn off server pagination' }),
+ ).not.toBeInTheDocument();
+});
+
+test.each([
+ ['semantic_view', ['ROW_OFFSET']],
+ ['table', undefined],
+ ['table', []],
+])('keeps %s pagination editable with %s', async (type, features) => {
+ const onChange = jest.fn();
+ renderPagination(panelState(features, type), onChange);
+ expect(screen.getByRole('checkbox')).toBeEnabled();
+ await userEvent.click(screen.getByRole('checkbox'));
+ expect(onChange).toHaveBeenCalledWith(true);
+ expect(
+ screen.queryByText(/does not support server pagination/),
+ ).not.toBeInTheDocument();
+});
+
+test('disables dependent page length without hiding or resetting saved state', () => {
+ const state = panelState([], 'semantic_view', true);
+ expect(
+ pageLength.mapStateToProps?.(state, state.controls.server_pagination),
+ ).toMatchObject({ disabled: true });
+ expect(
+ pageLength.visibility?.(
+ { ...state, actions: { setDatasource: jest.fn() }, exportState: {} },
+ {},
+ ),
+ ).toBe(true);
+ expect(state.controls.server_pagination.value).toBe(true);
+});
+
+test('saved true survives datasource changes until an explicit keyboard edit', async () => {
+ const saved = Object.freeze({ server_pagination: true });
+ const onEdit = jest.fn();
+ function Editor({ features }: { features?: string[] }) {
+ const [value, setValue] = useState<boolean>(saved.server_pagination);
+ const state = panelState(features, 'semantic_view', value);
+ return (
+ <CheckboxControl
+ name="server_pagination"
+ label="Server pagination"
+ {...pagination.mapStateToProps?.(
+ state,
+ state.controls.server_pagination,
+ )}
+ value={value}
+ onChange={next => {
+ setValue(next);
+ onEdit(next);
+ }}
+ />
+ );
+ }
+ const { rerender } = render(<Editor features={['ROW_OFFSET']} />);
+ expect(screen.getByRole('checkbox')).toBeChecked();
+ rerender(<Editor features={[]} />);
+ expect(screen.getByRole('checkbox')).toBeChecked();
+ expect(screen.getByRole('checkbox')).toBeDisabled();
+ expect(onEdit).not.toHaveBeenCalled();
+ await userEvent.tab();
+ expect(
+ screen.getByRole('button', { name: 'Turn off server pagination' }),
+ ).toHaveFocus();
+ await userEvent.type(
Review Comment:
Good catch—fixed in dfe94778cfac1c91d28f167b910c7ffa4f3f7a79 by using userEvent.keyboard("{Enter}") after the existing Tab/focus assertion. I verified the gap with a temporary mutant that prevents Enter activation but leaves mouse clicks working: the old test passed, the corrected test failed, and restoring the component made both focused suites pass (54 tests). The change is test-only.
--
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] feat(semantic-layer): gate Table pagination on row offset support [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #44452:
URL: https://github.com/apache/superset/pull/44452#issuecomment-5745939785
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/44452?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 80.69%. Comparing base ([`43fee87`](https://app.codecov.io/gh/apache/superset/commit/43fee87672297d2217ad297b1e80042a4358074b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`924b94f`](https://app.codecov.io/gh/apache/superset/commit/924b94fdc83da47e5fd727fe8500ad4313e5d2e0?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #44452 +/- ##
==========================================
+ Coverage 80.65% 80.69% +0.03%
==========================================
Files 2942 2947 +5
Lines 175692 176316 +624
Branches 40788 40788
==========================================
+ Hits 141705 142270 +565
- Misses 31325 31384 +59
Partials 2662 2662
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/44452/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [superset-extensions-cli](https://app.codecov.io/gh/apache/superset/pull/44452/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `90.57% <ø> (?)` | |
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 Harness](https://app.codecov.io/gh/apache/superset/pull/44452?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] feat(semantic-layer): gate Table pagination on row offset support [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #44452:
URL: https://github.com/apache/superset/pull/44452#issuecomment-5745939948
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 924b94fdc83da47e5fd727fe8500ad4313e5d2e0 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6aaf127998aae300081728a5 |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-44452--superset-docs-preview.netlify.app](https://deploy-preview-44452--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> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DcpBCoAgEADAv-y5BbVS6wc9YzUloVoxISL6e97mMC_kwkeuMAN04Aqdfmu-PEolh15j4Rs5xitU9JTJpT3Vp9U15J2fZW1ZE0WpzDRZotALIaw0ytII3w8) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-44452--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] feat(semantic-layer): gate Table pagination on row offset support [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44452:
URL: https://github.com/apache/superset/pull/44452#issuecomment-5921081446
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 an incremental AI Review.
- `/review full` - 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] feat(semantic-layer): gate Table pagination on row offset support [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #44452:
URL: https://github.com/apache/superset/pull/44452#discussion_r4263259426
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/test/controlPanel.test.tsx:
##########
@@ -327,3 +333,249 @@ test('metrics control includes non-filterable columns', () => {
]),
);
});
+
+test.each([
+ ['semantic_view', undefined, true],
+ ['semantic_view', [], true],
+ ['semantic_view', ['UNKNOWN'], true],
+ ['semantic_view', ['ROW_OFFSET'], false],
+ ['table', undefined, false],
+])(
+ 'AG Grid pagination gate: %s with %s disables=%s',
+ (type, features, disabled) => {
+ const panel = getPaginationControl('server_pagination');
+ const length = getPaginationControl('server_page_length');
+ const base = createMockExplore(undefined);
+ const state: ControlPanelState = {
+ ...base,
+ datasource: {
+ id: 1,
+ uid: 'provider-view',
+ type,
+ semantic_view_features: features,
+ } as Dataset,
+ form_data: {
+ ...base.form_data,
+ datasource: `1__${type}`,
+ server_pagination: true,
+ },
+ controls: {
+ ...base.controls,
+ server_pagination: { type: 'CheckboxControl', value: true },
+ },
+ };
+
+ expect(
+ panel.shouldMapStateToProps?.(
+ state,
+ state,
+ state.controls.server_pagination,
+ ),
+ ).toBe(true);
+ expect(
+ panel.mapStateToProps?.(state, state.controls.server_pagination),
+ ).toMatchObject({
+ disabled,
+ resetLabel: 'Turn off server pagination',
+ disabledReason: 'This semantic view does not support server pagination.',
+ });
+ expect(
+ length.mapStateToProps?.(state, state.controls.server_page_length),
+ ).toMatchObject({ disabled });
+ expect(state.controls.server_pagination.value).toBe(true);
Review Comment:
This asserts on the fixture's own `state.controls.server_pagination.value`, not on anything the new `mapStateToProps` returns, and `toMatchObject` ignores extra keys. If the AG Grid mapper started returning `value: false` for an unsupported view, a saved enabled setting would uncheck itself and every case here would still pass. Could this render the checkbox with the mapped props, as the Table panel test now does, and assert it stays checked?
--
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] feat(semantic-layer): gate Table pagination on row offset support [superset]
Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
mikebridge commented on code in PR #44452:
URL: https://github.com/apache/superset/pull/44452#discussion_r4264144287
##########
superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx:
##########
@@ -438,6 +440,16 @@ const config: ControlPanelConfig = {
'Enable server side pagination of results (experimental feature)',
),
default: false,
+ shouldMapStateToProps: () => true,
+ mapStateToProps: state => ({
+ disabled: isServerPaginationUnsupported(state),
+ disabledReason: hasMatchingDatasourceMetadata(state)
+ ? t('This semantic view does not support server pagination.')
+ : undefined,
+ resetLabel: hasMatchingDatasourceMetadata(state)
Review Comment:
Applied in [7e17676b4d](https://github.com/apache/superset/commit/7e17676b4ddf61ddd9220fb737eacd77d080ef05) for both Table and AG Grid. Unsupported views now offer row-limit choices only up to the effective SQL_MAX_ROW. After turning pagination off, an excessive limit produces “Lower the row limit to SQL_MAX_ROW (…) or less” instead of asking to enable the disabled checkbox. Saved enabled settings and limits remain untouched; SQL datasets and ROW_OFFSET-capable providers retain their existing behavior. Red-first checks cover both panels with a configured cap and the default cap, and all 272 focused tests plus changed-file/branch hooks pass. @aminghadersohi, this changes frontend code following your approval, so please re-look at the delta. The UX choice is also recorded for Mike to confirm in the conductor result. Implemented and verified by Codex; independently reviewed and code-approved by Claude Fable 5.1.
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/test/controlPanel.test.tsx:
##########
@@ -327,3 +333,249 @@ test('metrics control includes non-filterable columns', () => {
]),
);
});
+
+test.each([
+ ['semantic_view', undefined, true],
+ ['semantic_view', [], true],
+ ['semantic_view', ['UNKNOWN'], true],
+ ['semantic_view', ['ROW_OFFSET'], false],
+ ['table', undefined, false],
+])(
+ 'AG Grid pagination gate: %s with %s disables=%s',
+ (type, features, disabled) => {
+ const panel = getPaginationControl('server_pagination');
+ const length = getPaginationControl('server_page_length');
+ const base = createMockExplore(undefined);
+ const state: ControlPanelState = {
+ ...base,
+ datasource: {
+ id: 1,
+ uid: 'provider-view',
+ type,
+ semantic_view_features: features,
+ } as Dataset,
+ form_data: {
+ ...base.form_data,
+ datasource: `1__${type}`,
+ server_pagination: true,
+ },
+ controls: {
+ ...base.controls,
+ server_pagination: { type: 'CheckboxControl', value: true },
+ },
+ };
+
+ expect(
+ panel.shouldMapStateToProps?.(
+ state,
+ state,
+ state.controls.server_pagination,
+ ),
+ ).toBe(true);
+ expect(
+ panel.mapStateToProps?.(state, state.controls.server_pagination),
+ ).toMatchObject({
+ disabled,
+ resetLabel: 'Turn off server pagination',
+ disabledReason: 'This semantic view does not support server pagination.',
+ });
+ expect(
+ length.mapStateToProps?.(state, state.controls.server_page_length),
+ ).toMatchObject({ disabled });
+ expect(state.controls.server_pagination.value).toBe(true);
Review Comment:
Fixed in [7e17676b4d](https://github.com/apache/superset/commit/7e17676b4ddf61ddd9220fb737eacd77d080ef05). The AG Grid matrix now renders the actual CheckboxControl with mapped props applied last, matching the real control-panel precedence, and checks that the rendered checkbox remains checked and has the expected disabled state. A temporary unsupported-view `value: false` mapper mutation passes the old matrix but fails all three unsupported cases in the corrected matrix. Production is restored; all 272 focused tests and required hooks, including TypeScript checking, pass. Implemented and verified by Codex; independently reviewed and approved by Claude Fable 5.1.
--
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] feat(semantic-layer): gate Table pagination on row offset support [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #44452:
URL: https://github.com/apache/superset/pull/44452#discussion_r4149212505
##########
superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx:
##########
@@ -60,6 +60,20 @@ import { GenericDataType } from '@apache-superset/core/common';
import { isEmpty, last } from 'lodash-es';
import { PAGE_SIZE_OPTIONS, SERVER_PAGE_SIZE_OPTIONS } from './consts';
+function isServerPaginationUnsupported({
Review Comment:
Verified the approval's AG Grid question: ag-grid-table's `server_pagination`/`server_page_length` (controlPanel.tsx:384,397) are ungated and its buildQuery emits `row_offset` (:249,271), so with `AG_GRID_TABLE_ENABLED` the same provider error is reachable. Share this helper, or note the deferral?
--
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] feat(semantic-layer): gate Table pagination on row offset support [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #44452:
URL: https://github.com/apache/superset/pull/44452#discussion_r4236260013
##########
superset-frontend/src/explore/components/controls/CheckboxControl.tsx:
##########
@@ -50,24 +58,77 @@ const CheckBoxControlWrapper = styled.div`
export default function CheckboxControl({
value = false,
label,
+ disabled = false,
+ disabledReason,
+ resetLabel,
onChange = () => {},
...restProps
}: CheckboxControlProps): JSX.Element {
+ const explanationId = useId();
+ const generatedCheckboxId = useId();
+ const checkboxId = restProps.name || generatedCheckboxId;
Review Comment:
Now that the checkbox input gets `id={name}`, controls that reuse a static `name` across several mounted instances end up with duplicate DOM ids. The annotation layer editors do this (`annotation-layer-show-markers`, `annotation-layer-hide-line`, `annotation-override-time_range`, ...), and `ControlPopover` leaves closed popovers mounted (`destroyOnHidden` defaults to false). After opening layer A and then layer B, B's header label `htmlFor` resolves to A's hidden input, and the new `document.getElementById(checkboxId)?.focus()` on label click also lands on A's input, so B's checkbox ends up with the wrong accessible-name association and focus goes to a hidden element. Should the input id be scoped per instance (for example by always using the generated id for the input and only forwarding `name` for the `data-test` attributes)?
--
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] feat(semantic-layer): gate Table pagination on row offset support [superset]
Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
mikebridge commented on code in PR #44452:
URL: https://github.com/apache/superset/pull/44452#discussion_r4259726136
##########
superset-frontend/plugins/plugin-chart-table/test/controlPanel.test.tsx:
##########
@@ -473,6 +484,403 @@ test('columnOptions defaults type_generic to String when missing from datasource
);
});
+function getControl(name: string): ControlConfig {
+ for (const section of config.controlPanelSections) {
+ if (!section) continue;
+ for (const row of section.controlSetRows) {
+ for (const control of row) {
+ if (isCustomControlItem(control) && control.name === name) {
+ return control.config;
+ }
+ }
+ }
+ }
+ throw new Error(`Missing control: ${name}`);
+}
+
+const pagination = getControl('server_pagination');
+const pageLength = getControl('server_page_length');
+
+function panelState(
+ features?: string[],
+ type = 'semantic_view',
+ value = false,
+): ControlPanelState {
+ return {
+ datasource: {
+ id: 1,
+ uid: 'provider-view',
+ type,
+ semantic_view_features: features,
+ } as Dataset,
+ form_data: {
+ datasource: `1__${type}`,
+ viz_type: 'table',
+ server_pagination: value,
+ },
+ controls: { server_pagination: { type: 'CheckboxControl', value } },
+ slice: { slice_id: 1 },
+ common: {},
+ };
+}
+
+test('recomputes pagination controls when datasource metadata changes', () => {
+ const registry = getChartControlPanelRegistry();
+ registry.registerValue('table', {
+ controlPanelSections: [
+ {
+ label: 'Options',
+ expanded: true,
+ controlSetRows: [
+ [{ name: 'server_pagination', config: pagination }],
+ [{ name: 'server_page_length', config: pageLength }],
+ ],
+ },
+ ],
+ });
+ const state = panelState(['ROW_OFFSET']);
+ const props = {
+ actions: { setControlValue: jest.fn() },
+ chart: { chartStatus: 'success', queriesResponse: null },
+ controls: {
+ ...state.controls,
+ server_page_length: { type: 'SelectControl', value: 10 },
+ },
+ datasource_type: DatasourceType.SemanticView,
+ exploreState: state,
+ form_data: state.form_data,
+ isDatasourceMetaLoading: false,
+ errorMessage: null,
+ onQuery: jest.fn(),
+ onStop: jest.fn(),
+ canStopQuery: false,
+ chartIsStale: false,
+ } as unknown as ControlPanelsContainerProps;
+
+ try {
+ const { rerender } = render(<ControlPanelsContainer {...props} />, {
+ useRedux: true,
+ });
+ expect(screen.getByRole('checkbox')).toBeEnabled();
+ rerender(
+ <ControlPanelsContainer
+ {...props}
+ exploreState={{
+ ...props.exploreState,
+ datasource: panelState([]).datasource as Dataset,
+ }}
+ />,
+ );
+ expect(screen.getByRole('checkbox')).toBeDisabled();
+ } finally {
+ registry.remove('table');
+ }
+});
+
+test.each([
+ ['server_pagination', pagination],
+ ['server_page_length', pageLength],
+])('%s remaps when datasource capabilities change', (name, control) => {
+ const before = panelState(['ROW_OFFSET']);
+ const after = panelState([]);
+ const controlState = after.controls[name];
+
+ expect(control.shouldMapStateToProps?.(before, after, controlState)).toBe(
+ true,
+ );
+ expect(control.mapStateToProps?.(after, controlState)).toMatchObject({
+ disabled: true,
+ });
+});
+
+function renderPagination(state: ControlPanelState, onChange = jest.fn()) {
+ return render(
+ <CheckboxControl
+ name="server_pagination"
+ label="Server pagination"
+ {...pagination.mapStateToProps?.(state, state.controls.server_pagination)}
+ value={Boolean(state.controls.server_pagination.value)}
+ onChange={onChange}
+ />,
+ );
+}
+
+test.each([undefined, [], ['UNKNOWN'], ['row_offset']])(
+ 'disables unsupported semantic pagination with explanation: %s',
+ async features => {
+ const onChange = jest.fn();
+ const state = panelState(features);
+ renderPagination(state, onChange);
+ const checkbox = screen.getByRole('checkbox');
+ expect(pagination.type).toBe('CheckboxControl');
+ expect(checkbox).toBeDisabled();
+ expect(checkbox).toHaveAccessibleDescription(
+ 'This semantic view does not support server pagination.',
+ );
+ await userEvent.click(screen.getByText('Server pagination'));
+ expect(onChange).not.toHaveBeenCalled();
+ },
+);
+
+test('fails closed while semantic datasource metadata is unavailable', () => {
+ const state = { ...panelState(), datasource: null };
+ renderPagination(state);
+ expect(screen.getByRole('checkbox')).toBeDisabled();
+});
+
+test.each([
+ ['1__semantic_view', '2__semantic_view'],
+ ['cube__orders', 'cube__customers'],
+])(
+ 'withholds pagination advice for stale metadata %s -> %s',
+ (uid, selected) => {
+ const state = panelState([], 'semantic_view', true);
+ state.datasource = {
+ ...state.datasource,
+ id: 1,
+ uid,
+ type: 'semantic_view',
+ semantic_view_features: [],
+ } as Dataset;
+ state.form_data.datasource = selected;
+ const panel = getControl('server_pagination');
+ expect(
+ panel.mapStateToProps?.(state, state.controls.server_pagination),
+ ).toMatchObject({
+ disabled: true,
+ resetLabel: undefined,
+ disabledReason: undefined,
+ });
+ state.datasource = { ...state.datasource, id: 2, uid: selected } as Dataset;
+ expect(
+ panel.mapStateToProps?.(state, state.controls.server_pagination),
+ ).toMatchObject({
+ disabled: true,
+ resetLabel: 'Turn off server pagination',
+ disabledReason: 'This semantic view does not support server pagination.',
+ });
+ },
+);
+
+test.each([true, false])(
+ 'matches Explore id/type independently of provider uid: offset=%s',
+ supportsOffset => {
+ const state = panelState([], 'semantic_view', true);
+ state.datasource = {
+ ...state.datasource,
+ id: 42,
+ uid: 'provider-orders',
+ type: 'semantic_view',
+ semantic_view_features: supportsOffset ? ['ROW_OFFSET'] : [],
+ } as Dataset;
+ state.form_data.datasource = '42__semantic_view';
+ const panel = getControl('server_pagination');
+ expect(
+ panel.mapStateToProps?.(state, state.controls.server_pagination),
+ ).toMatchObject({
+ disabled: !supportsOffset,
+ resetLabel: 'Turn off server pagination',
+ disabledReason: 'This semantic view does not support server pagination.',
+ });
+ state.datasource = {
+ ...state.datasource,
+ id: 43,
+ uid: '42__semantic_view',
+ } as Dataset;
+ expect(
+ panel.mapStateToProps?.(state, state.controls.server_pagination),
+ ).toMatchObject({
+ disabled: true,
+ resetLabel: undefined,
+ disabledReason: undefined,
+ });
+ state.datasource = {
+ ...state.datasource,
+ id: 42,
+ type: 'table',
+ } as Dataset;
+ expect(
+ panel.mapStateToProps?.(state, state.controls.server_pagination),
+ ).toMatchObject({
+ disabled: true,
+ resetLabel: undefined,
+ disabledReason: undefined,
+ });
+ },
+);
+
+test.each([undefined, [], ['ROW_OFFSET']])(
+ 'matches opaque provider uid before parsing datasource type: %s',
+ features => {
+ const state = panelState(features);
+ state.datasource = { ...state.datasource, uid: 'cube__orders' } as Dataset;
+ state.form_data.datasource = 'cube__orders';
+ renderPagination(state);
+ const disabled = !features?.includes('ROW_OFFSET');
+ expect(screen.getByRole('checkbox').hasAttribute('disabled')).toBe(
+ disabled,
+ );
+ expect(
+ pageLength.mapStateToProps?.(state, state.controls.server_page_length),
+ ).toMatchObject({ disabled });
+ },
+);
+
+test.each([
+ ['table', 'semantic_view', true],
+ ['semantic_view', 'table', false],
+])(
+ 'trusts form datasource %s -> %s while metadata is stale',
+ (previous, next, disabled) => {
+ const state = panelState([], previous, true);
+ state.form_data.datasource = `2__${next}`;
+ renderPagination(state);
+ expect(screen.getByRole('checkbox').hasAttribute('disabled')).toBe(
+ disabled,
+ );
+ expect(
+ pageLength.mapStateToProps?.(state, state.controls.server_pagination),
+ ).toMatchObject({ disabled });
+ expect(state.controls.server_pagination.value).toBe(true);
+ },
+);
+
+test('ignores stale offset capability when switching opaque semantic UIDs', () => {
+ const state = panelState(['ROW_OFFSET'], 'semantic_view', true);
+ state.datasource = {
+ ...state.datasource,
+ uid: 'cube__orders',
+ type: 'semantic_view',
+ semantic_view_features: ['ROW_OFFSET'],
+ } as Dataset;
+ state.form_data.datasource = 'cube__customers';
+
+ for (const name of ['server_pagination', 'server_page_length']) {
+ expect(
+ getControl(name).mapStateToProps?.(state, state.controls[name]),
+ ).toMatchObject({ disabled: true });
+ }
+});
+
+test.each(['server_pagination', 'server_page_length'])(
+ '%s ignores stale offset capability from another semantic view',
+ name => {
+ const state = panelState(['ROW_OFFSET'], 'semantic_view', true);
+ state.datasource = {
+ ...state.datasource,
+ uid: '1__semantic_view',
+ } as Dataset;
+ state.form_data.datasource = '2__semantic_view';
+
+ expect(
+ getControl(name).mapStateToProps?.(state, state.controls[name]),
+ ).toMatchObject({ disabled: true });
+ expect(state.controls.server_pagination.value).toBe(true);
+ },
+);
+
+test('does not offer a reset before datasource metadata arrives', () => {
+ const state = {
+ ...panelState(undefined, 'semantic_view', true),
+ datasource: null,
+ };
+ renderPagination(state);
+ expect(
+ getControl('server_pagination').mapStateToProps?.(
+ state,
+ state.controls.server_pagination,
+ ),
+ ).toMatchObject({ disabledReason: undefined, resetLabel: undefined });
+ expect(screen.getByRole('checkbox')).toBeDisabled();
+ expect(screen.getByRole('checkbox')).toBeChecked();
+ expect(
+ screen.queryByRole('button', { name: 'Turn off server pagination' }),
+ ).not.toBeInTheDocument();
+});
+
+test.each([
+ ['semantic_view', ['ROW_OFFSET']],
+ ['table', undefined],
+ ['table', []],
+])('keeps %s pagination editable with %s', async (type, features) => {
+ const onChange = jest.fn();
+ renderPagination(panelState(features, type), onChange);
+ expect(screen.getByRole('checkbox')).toBeEnabled();
+ await userEvent.click(screen.getByRole('checkbox'));
+ expect(onChange).toHaveBeenCalledWith(true);
+ expect(
+ screen.queryByText(/does not support server pagination/),
+ ).not.toBeInTheDocument();
+});
+
+test('disables dependent page length without hiding or resetting saved state', () => {
+ const state = panelState([], 'semantic_view', true);
+ expect(
+ pageLength.mapStateToProps?.(state, state.controls.server_pagination),
+ ).toMatchObject({ disabled: true });
+ expect(
+ pageLength.visibility?.(
+ { ...state, actions: { setDatasource: jest.fn() }, exportState: {} },
+ {},
+ ),
+ ).toBe(true);
+ expect(state.controls.server_pagination.value).toBe(true);
+});
+
+test.each(['semantic_view', 'table'])(
+ 'checked enabled %s pagination has no unsupported-control reset button',
+ type => {
+ renderPagination(panelState(['ROW_OFFSET'], type, true));
+ expect(screen.getByRole('checkbox')).toBeEnabled();
+ expect(screen.getByRole('checkbox')).toBeChecked();
+ expect(
+ screen.queryByRole('button', { name: 'Turn off server pagination' }),
+ ).not.toBeInTheDocument();
+ },
+);
+
+test('saved true survives datasource changes until an explicit keyboard edit', async () => {
+ const saved = Object.freeze({ server_pagination: true });
+ const onEdit = jest.fn();
+ function Editor({ features }: { features?: string[] }) {
+ const [value, setValue] = useState<boolean>(saved.server_pagination);
+ const state = panelState(features, 'semantic_view', value);
+ return (
+ <CheckboxControl
+ name="server_pagination"
+ label="Server pagination"
+ {...pagination.mapStateToProps?.(
+ state,
+ state.controls.server_pagination,
+ )}
+ value={value}
Review Comment:
Verified against `ControlPanelsContainer`: mapped props are merged after the saved control state, and this pagination control recalculates them each render. You are right that the test's local value masked a mapped override. Fixed in [6b37cc4471](https://github.com/apache/superset/commit/6b37cc447152d6f6549b17654c033c79b5e2debd) with the direct assertion you suggested: an unsupported semantic view with saved pagination enabled must return mapped props with no `value` key. This avoids the generic mapped-props type needing a cast while pinning the actual saved-value contract. I temporarily made the unsupported branch return `value: false`: the unchanged old test passed, and the corrected test failed at the new assertion. Production code is restored. All 86 tests across the table, ag-grid and checkbox suites pass, along with changed-file and branch hooks. Test fix and verification by Codex; independently reviewed and approved by Claude Fable 5.1.
--
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] feat(semantic-layer): gate Table pagination on row offset support [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #44452:
URL: https://github.com/apache/superset/pull/44452#discussion_r4253568955
##########
superset-frontend/packages/superset-ui-chart-controls/src/utils/isServerPaginationUnsupported.ts:
##########
@@ -0,0 +1,68 @@
+/**
+ * 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 { DatasourceType } from '@superset-ui/core';
+import type { ControlPanelState } from '../types';
+
+/** SemanticViewFeature value for providers that support row offsets. */
+export const ROW_OFFSET = 'ROW_OFFSET';
+
+/** Match Explore's host identity, or an opaque provider selection. */
+export function hasMatchingDatasourceMetadata({
+ datasource,
+ form_data,
+}: Pick<ControlPanelState, 'datasource' | 'form_data'>): boolean {
+ const selected = form_data.datasource?.match(/^(\d+)__(.+)$/);
Review Comment:
A provider uid that starts with digits (e.g. `123__orders`, which the reducer writes straight into `form_data.datasource` on a datasource switch) matches this regex as host id `123`, type `orders`. The id/type comparison then fails against the metadata (`id: 7`) and the uid equality below is never tried, so a view that advertises `ROW_OFFSET` gets pagination disabled with no explanation or reset action; the same mismatch hides the reset when the capability is absent. Could the uid-equality check run before the id/type parse, or the regex be limited to the recognized datasource types?
##########
superset-frontend/src/explore/components/ControlHeader.tsx:
##########
@@ -162,23 +164,27 @@ const ControlHeader: FC<ControlHeaderProps> = ({
overflow: visible;
padding-bottom: 0.1em;
`}
- htmlFor={name}
+ htmlFor={inputId ?? name}
>
{leftNode && <span>{leftNode} </span>}
{/* This label text sits inside FormLabel's <label>, and HTML5
prohibits interactive content (including <button>) as a
descendant of <label>, so a real button tag isn't an option
here. */}
- <span
- // eslint-disable-next-line jsx-a11y/prefer-tag-over-role
- role="button"
- tabIndex={0}
- onClick={onClick}
- onKeyDown={onClick ? handleKeyboardActivation(onClick) : undefined}
- style={{ cursor: onClick ? 'pointer' : '' }}
- >
- {label}
- </span>{' '}
+ {onClick ? (
+ <span
+ // eslint-disable-next-line jsx-a11y/prefer-tag-over-role
+ role="button"
+ tabIndex={0}
+ onClick={onClick}
+ onKeyDown={handleKeyboardActivation(onClick)}
+ style={{ cursor: 'pointer' }}
+ >
+ {label}
+ </span>
+ ) : (
+ <span>{label}</span>
Review Comment:
Without the header's tab stop, a control whose input can't take focus (a disabled Select such as Server Page Length, or a dashboard-locked color scheme) has nothing for keyboard users to focus, so `hovered` never turns on and the description button in `renderOptionalIcons` never renders. Its help text is then mouse-only, which it wasn't when the label was tabbable. Is that acceptable for disabled controls, or should the description trigger be reachable without relying on hover/focus of the input?
--
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] feat(semantic-layer): gate Table pagination on row offset support [superset]
Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
mikebridge commented on code in PR #44452:
URL: https://github.com/apache/superset/pull/44452#discussion_r4253928646
##########
superset-frontend/packages/superset-ui-chart-controls/src/utils/isServerPaginationUnsupported.ts:
##########
@@ -0,0 +1,68 @@
+/**
+ * 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 { DatasourceType } from '@superset-ui/core';
+import type { ControlPanelState } from '../types';
+
+/** SemanticViewFeature value for providers that support row offsets. */
+export const ROW_OFFSET = 'ROW_OFFSET';
+
+/** Match Explore's host identity, or an opaque provider selection. */
+export function hasMatchingDatasourceMetadata({
+ datasource,
+ form_data,
+}: Pick<ControlPanelState, 'datasource' | 'form_data'>): boolean {
+ const selected = form_data.datasource?.match(/^(\d+)__(.+)$/);
Review Comment:
Applied in [cf4b7925d5](https://github.com/apache/superset/commit/cf4b7925d51ee23ecc03e11b3f8579b207e3e96c). The id/type regex now recognizes only `table` and `semantic_view`, so `123__orders` reaches uid equality. This also preserves rejection of stale host metadata such as id 43 with uid `42__semantic_view`. Red-first tests cover both ROW_OFFSET capability states, page-length disabling, and reset visibility. Final verification: 155 tests across eight focused suites pass; changed-file and branch hooks pass. Implemented and verified by Codex, independently reviewed and approved by Claude Fable 5.1.
##########
superset-frontend/src/explore/components/ControlHeader.tsx:
##########
@@ -162,23 +164,27 @@ const ControlHeader: FC<ControlHeaderProps> = ({
overflow: visible;
padding-bottom: 0.1em;
`}
- htmlFor={name}
+ htmlFor={inputId ?? name}
>
{leftNode && <span>{leftNode} </span>}
{/* This label text sits inside FormLabel's <label>, and HTML5
prohibits interactive content (including <button>) as a
descendant of <label>, so a real button tag isn't an option
here. */}
- <span
- // eslint-disable-next-line jsx-a11y/prefer-tag-over-role
- role="button"
- tabIndex={0}
- onClick={onClick}
- onKeyDown={onClick ? handleKeyboardActivation(onClick) : undefined}
- style={{ cursor: onClick ? 'pointer' : '' }}
- >
- {label}
- </span>{' '}
+ {onClick ? (
+ <span
+ // eslint-disable-next-line jsx-a11y/prefer-tag-over-role
+ role="button"
+ tabIndex={0}
+ onClick={onClick}
+ onKeyDown={handleKeyboardActivation(onClick)}
+ style={{ cursor: 'pointer' }}
+ >
+ {label}
+ </span>
+ ) : (
+ <span>{label}</span>
Review Comment:
Applied in [cf4b7925d5](https://github.com/apache/superset/commit/cf4b7925d51ee23ecc03e11b3f8579b207e3e96c). The description button stays mounted and visible, so keyboard users can reach it even when the input is disabled; plain headers still have no redundant tab stop. A red-first test uses the actual disabled Server Page Length Select and tabs onto its help button. Each tooltip also gets a unique React ID, with a regression test verifying that tabbing between two controls announces the correct description. UPDATING documents the visible help buttons. All 155 focused frontend tests and changed-file/branch hooks pass. Implemented and verified by Codex, independently reviewed and approved by Claude Fable 5.1.
--
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] feat(semantic-layer): gate Table pagination on row offset support [superset]
Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
mikebridge commented on code in PR #44452:
URL: https://github.com/apache/superset/pull/44452#discussion_r4254681069
##########
superset-frontend/src/explore/components/controls/CheckboxControl.tsx:
##########
@@ -50,24 +58,77 @@ const CheckBoxControlWrapper = styled.div`
export default function CheckboxControl({
value = false,
label,
+ disabled = false,
+ disabledReason,
+ resetLabel,
onChange = () => {},
...restProps
}: CheckboxControlProps): JSX.Element {
+ const explanationId = useId();
+ const checkboxId = useId();
const handleChange = useCallback((): void => {
- onChange(!value);
- }, [onChange, value]);
+ if (!disabled) {
+ onChange(!value);
+ }
+ }, [disabled, onChange, value]);
- const checkbox = <Checkbox onChange={handleChange} checked={!!value} />;
+ const checkbox = (
+ <Checkbox
+ id={checkboxId}
+ onChange={handleChange}
+ checked={!!value}
+ disabled={disabled}
+ aria-describedby={disabled && disabledReason ? explanationId : undefined}
+ />
+ );
+ const explanation = disabled && disabledReason && (
+ <Typography.Text id={explanationId} type="secondary">
+ {disabledReason}
+ </Typography.Text>
+ );
+ const resetButton = disabled && value && resetLabel && (
Review Comment:
Added in [f81a09f485](https://github.com/apache/superset/commit/f81a09f485c3c6e8d9fa9848d0e724ff45f1fb41). Both checked-and-enabled cases—semantic_view with ROW_OFFSET and table—now assert that `queryByRole('button', { name: 'Turn off server pagination' })` is absent. Removing `disabled &&` from the reset-button guard makes both cases fail; restoring it passes. Final verification after the master merge: 384 frontend tests across 11 suites and 973 backend/SDK tests pass, with changed-file and branch hooks passing. Test additions and verification by Codex; independent review and approval by Claude Fable 5.1.
##########
superset-frontend/src/explore/components/ControlHeader.tsx:
##########
@@ -130,7 +133,7 @@ const ControlHeader: FC<ControlHeaderProps> = ({
</Tooltip>{' '}
</>
)}
- {renderTrigger && (
+ {hovered && renderTrigger && (
Review Comment:
Added in [f81a09f485](https://github.com/apache/superset/commit/f81a09f485c3c6e8d9fa9848d0e724ff45f1fb41). The new ControlHeader test renders both `description` and `renderTrigger`, checks that the bolt is absent with `hovered=false`, rerenders with `hovered=true` and checks that the bolt appears, and verifies description help remains present in both states. Removing `hovered &&` makes the new test fail; restoring it passes. Final verification: all 384 frontend tests across 11 suites and required hooks pass. Test additions and verification by Codex; independent review and approval by Claude Fable 5.1.
--
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] feat(semantic-layer): gate Table pagination on row offset support [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #44452:
URL: https://github.com/apache/superset/pull/44452#discussion_r4255238049
##########
superset-frontend/plugins/plugin-chart-table/test/controlPanel.test.tsx:
##########
@@ -473,6 +484,403 @@ test('columnOptions defaults type_generic to String when missing from datasource
);
});
+function getControl(name: string): ControlConfig {
+ for (const section of config.controlPanelSections) {
+ if (!section) continue;
+ for (const row of section.controlSetRows) {
+ for (const control of row) {
+ if (isCustomControlItem(control) && control.name === name) {
+ return control.config;
+ }
+ }
+ }
+ }
+ throw new Error(`Missing control: ${name}`);
+}
+
+const pagination = getControl('server_pagination');
+const pageLength = getControl('server_page_length');
+
+function panelState(
+ features?: string[],
+ type = 'semantic_view',
+ value = false,
+): ControlPanelState {
+ return {
+ datasource: {
+ id: 1,
+ uid: 'provider-view',
+ type,
+ semantic_view_features: features,
+ } as Dataset,
+ form_data: {
+ datasource: `1__${type}`,
+ viz_type: 'table',
+ server_pagination: value,
+ },
+ controls: { server_pagination: { type: 'CheckboxControl', value } },
+ slice: { slice_id: 1 },
+ common: {},
+ };
+}
+
+test('recomputes pagination controls when datasource metadata changes', () => {
+ const registry = getChartControlPanelRegistry();
+ registry.registerValue('table', {
+ controlPanelSections: [
+ {
+ label: 'Options',
+ expanded: true,
+ controlSetRows: [
+ [{ name: 'server_pagination', config: pagination }],
+ [{ name: 'server_page_length', config: pageLength }],
+ ],
+ },
+ ],
+ });
+ const state = panelState(['ROW_OFFSET']);
+ const props = {
+ actions: { setControlValue: jest.fn() },
+ chart: { chartStatus: 'success', queriesResponse: null },
+ controls: {
+ ...state.controls,
+ server_page_length: { type: 'SelectControl', value: 10 },
+ },
+ datasource_type: DatasourceType.SemanticView,
+ exploreState: state,
+ form_data: state.form_data,
+ isDatasourceMetaLoading: false,
+ errorMessage: null,
+ onQuery: jest.fn(),
+ onStop: jest.fn(),
+ canStopQuery: false,
+ chartIsStale: false,
+ } as unknown as ControlPanelsContainerProps;
+
+ try {
+ const { rerender } = render(<ControlPanelsContainer {...props} />, {
+ useRedux: true,
+ });
+ expect(screen.getByRole('checkbox')).toBeEnabled();
+ rerender(
+ <ControlPanelsContainer
+ {...props}
+ exploreState={{
+ ...props.exploreState,
+ datasource: panelState([]).datasource as Dataset,
+ }}
+ />,
+ );
+ expect(screen.getByRole('checkbox')).toBeDisabled();
+ } finally {
+ registry.remove('table');
+ }
+});
+
+test.each([
+ ['server_pagination', pagination],
+ ['server_page_length', pageLength],
+])('%s remaps when datasource capabilities change', (name, control) => {
+ const before = panelState(['ROW_OFFSET']);
+ const after = panelState([]);
+ const controlState = after.controls[name];
+
+ expect(control.shouldMapStateToProps?.(before, after, controlState)).toBe(
+ true,
+ );
+ expect(control.mapStateToProps?.(after, controlState)).toMatchObject({
+ disabled: true,
+ });
+});
+
+function renderPagination(state: ControlPanelState, onChange = jest.fn()) {
+ return render(
+ <CheckboxControl
+ name="server_pagination"
+ label="Server pagination"
+ {...pagination.mapStateToProps?.(state, state.controls.server_pagination)}
+ value={Boolean(state.controls.server_pagination.value)}
+ onChange={onChange}
+ />,
+ );
+}
+
+test.each([undefined, [], ['UNKNOWN'], ['row_offset']])(
+ 'disables unsupported semantic pagination with explanation: %s',
+ async features => {
+ const onChange = jest.fn();
+ const state = panelState(features);
+ renderPagination(state, onChange);
+ const checkbox = screen.getByRole('checkbox');
+ expect(pagination.type).toBe('CheckboxControl');
+ expect(checkbox).toBeDisabled();
+ expect(checkbox).toHaveAccessibleDescription(
+ 'This semantic view does not support server pagination.',
+ );
+ await userEvent.click(screen.getByText('Server pagination'));
+ expect(onChange).not.toHaveBeenCalled();
+ },
+);
+
+test('fails closed while semantic datasource metadata is unavailable', () => {
+ const state = { ...panelState(), datasource: null };
+ renderPagination(state);
+ expect(screen.getByRole('checkbox')).toBeDisabled();
+});
+
+test.each([
+ ['1__semantic_view', '2__semantic_view'],
+ ['cube__orders', 'cube__customers'],
+])(
+ 'withholds pagination advice for stale metadata %s -> %s',
+ (uid, selected) => {
+ const state = panelState([], 'semantic_view', true);
+ state.datasource = {
+ ...state.datasource,
+ id: 1,
+ uid,
+ type: 'semantic_view',
+ semantic_view_features: [],
+ } as Dataset;
+ state.form_data.datasource = selected;
+ const panel = getControl('server_pagination');
+ expect(
+ panel.mapStateToProps?.(state, state.controls.server_pagination),
+ ).toMatchObject({
+ disabled: true,
+ resetLabel: undefined,
+ disabledReason: undefined,
+ });
+ state.datasource = { ...state.datasource, id: 2, uid: selected } as Dataset;
+ expect(
+ panel.mapStateToProps?.(state, state.controls.server_pagination),
+ ).toMatchObject({
+ disabled: true,
+ resetLabel: 'Turn off server pagination',
+ disabledReason: 'This semantic view does not support server pagination.',
+ });
+ },
+);
+
+test.each([true, false])(
+ 'matches Explore id/type independently of provider uid: offset=%s',
+ supportsOffset => {
+ const state = panelState([], 'semantic_view', true);
+ state.datasource = {
+ ...state.datasource,
+ id: 42,
+ uid: 'provider-orders',
+ type: 'semantic_view',
+ semantic_view_features: supportsOffset ? ['ROW_OFFSET'] : [],
+ } as Dataset;
+ state.form_data.datasource = '42__semantic_view';
+ const panel = getControl('server_pagination');
+ expect(
+ panel.mapStateToProps?.(state, state.controls.server_pagination),
+ ).toMatchObject({
+ disabled: !supportsOffset,
+ resetLabel: 'Turn off server pagination',
+ disabledReason: 'This semantic view does not support server pagination.',
+ });
+ state.datasource = {
+ ...state.datasource,
+ id: 43,
+ uid: '42__semantic_view',
+ } as Dataset;
+ expect(
+ panel.mapStateToProps?.(state, state.controls.server_pagination),
+ ).toMatchObject({
+ disabled: true,
+ resetLabel: undefined,
+ disabledReason: undefined,
+ });
+ state.datasource = {
+ ...state.datasource,
+ id: 42,
+ type: 'table',
+ } as Dataset;
+ expect(
+ panel.mapStateToProps?.(state, state.controls.server_pagination),
+ ).toMatchObject({
+ disabled: true,
+ resetLabel: undefined,
+ disabledReason: undefined,
+ });
+ },
+);
+
+test.each([undefined, [], ['ROW_OFFSET']])(
+ 'matches opaque provider uid before parsing datasource type: %s',
+ features => {
+ const state = panelState(features);
+ state.datasource = { ...state.datasource, uid: 'cube__orders' } as Dataset;
+ state.form_data.datasource = 'cube__orders';
+ renderPagination(state);
+ const disabled = !features?.includes('ROW_OFFSET');
+ expect(screen.getByRole('checkbox').hasAttribute('disabled')).toBe(
+ disabled,
+ );
+ expect(
+ pageLength.mapStateToProps?.(state, state.controls.server_page_length),
+ ).toMatchObject({ disabled });
+ },
+);
+
+test.each([
+ ['table', 'semantic_view', true],
+ ['semantic_view', 'table', false],
+])(
+ 'trusts form datasource %s -> %s while metadata is stale',
+ (previous, next, disabled) => {
+ const state = panelState([], previous, true);
+ state.form_data.datasource = `2__${next}`;
+ renderPagination(state);
+ expect(screen.getByRole('checkbox').hasAttribute('disabled')).toBe(
+ disabled,
+ );
+ expect(
+ pageLength.mapStateToProps?.(state, state.controls.server_pagination),
+ ).toMatchObject({ disabled });
+ expect(state.controls.server_pagination.value).toBe(true);
+ },
+);
+
+test('ignores stale offset capability when switching opaque semantic UIDs', () => {
+ const state = panelState(['ROW_OFFSET'], 'semantic_view', true);
+ state.datasource = {
+ ...state.datasource,
+ uid: 'cube__orders',
+ type: 'semantic_view',
+ semantic_view_features: ['ROW_OFFSET'],
+ } as Dataset;
+ state.form_data.datasource = 'cube__customers';
+
+ for (const name of ['server_pagination', 'server_page_length']) {
+ expect(
+ getControl(name).mapStateToProps?.(state, state.controls[name]),
+ ).toMatchObject({ disabled: true });
+ }
+});
+
+test.each(['server_pagination', 'server_page_length'])(
+ '%s ignores stale offset capability from another semantic view',
+ name => {
+ const state = panelState(['ROW_OFFSET'], 'semantic_view', true);
+ state.datasource = {
+ ...state.datasource,
+ uid: '1__semantic_view',
+ } as Dataset;
+ state.form_data.datasource = '2__semantic_view';
+
+ expect(
+ getControl(name).mapStateToProps?.(state, state.controls[name]),
+ ).toMatchObject({ disabled: true });
+ expect(state.controls.server_pagination.value).toBe(true);
+ },
+);
+
+test('does not offer a reset before datasource metadata arrives', () => {
+ const state = {
+ ...panelState(undefined, 'semantic_view', true),
+ datasource: null,
+ };
+ renderPagination(state);
+ expect(
+ getControl('server_pagination').mapStateToProps?.(
+ state,
+ state.controls.server_pagination,
+ ),
+ ).toMatchObject({ disabledReason: undefined, resetLabel: undefined });
+ expect(screen.getByRole('checkbox')).toBeDisabled();
+ expect(screen.getByRole('checkbox')).toBeChecked();
+ expect(
+ screen.queryByRole('button', { name: 'Turn off server pagination' }),
+ ).not.toBeInTheDocument();
+});
+
+test.each([
+ ['semantic_view', ['ROW_OFFSET']],
+ ['table', undefined],
+ ['table', []],
+])('keeps %s pagination editable with %s', async (type, features) => {
+ const onChange = jest.fn();
+ renderPagination(panelState(features, type), onChange);
+ expect(screen.getByRole('checkbox')).toBeEnabled();
+ await userEvent.click(screen.getByRole('checkbox'));
+ expect(onChange).toHaveBeenCalledWith(true);
+ expect(
+ screen.queryByText(/does not support server pagination/),
+ ).not.toBeInTheDocument();
+});
+
+test('disables dependent page length without hiding or resetting saved state', () => {
+ const state = panelState([], 'semantic_view', true);
+ expect(
+ pageLength.mapStateToProps?.(state, state.controls.server_pagination),
+ ).toMatchObject({ disabled: true });
+ expect(
+ pageLength.visibility?.(
+ { ...state, actions: { setDatasource: jest.fn() }, exportState: {} },
+ {},
+ ),
+ ).toBe(true);
+ expect(state.controls.server_pagination.value).toBe(true);
+});
+
+test.each(['semantic_view', 'table'])(
+ 'checked enabled %s pagination has no unsupported-control reset button',
+ type => {
+ renderPagination(panelState(['ROW_OFFSET'], type, true));
+ expect(screen.getByRole('checkbox')).toBeEnabled();
+ expect(screen.getByRole('checkbox')).toBeChecked();
+ expect(
+ screen.queryByRole('button', { name: 'Turn off server pagination' }),
+ ).not.toBeInTheDocument();
+ },
+);
+
+test('saved true survives datasource changes until an explicit keyboard edit', async () => {
+ const saved = Object.freeze({ server_pagination: true });
+ const onEdit = jest.fn();
+ function Editor({ features }: { features?: string[] }) {
+ const [value, setValue] = useState<boolean>(saved.server_pagination);
+ const state = panelState(features, 'semantic_view', value);
+ return (
+ <CheckboxControl
+ name="server_pagination"
+ label="Server pagination"
+ {...pagination.mapStateToProps?.(
+ state,
+ state.controls.server_pagination,
+ )}
+ value={value}
Review Comment:
This test overwrites whatever `mapStateToProps` returns for `value` with its own `useState`, but in the real control panel the mapped props are merged last. If the unsupported-view branch ever started returning `value: false`, the saved checkbox would uncheck itself and the reset button would disappear in the app, while this test still passes. Could the test render with the mapped props winning (or assert that the mapped props contain no `value` key) so the "saved true is never rewritten" claim is actually pinned?
--
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] feat(semantic-layer): gate Table pagination on row offset support [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44452:
URL: https://github.com/apache/superset/pull/44452#issuecomment-6009162511
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 an incremental AI Review.
- `/review full` - 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