You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "kokhlo (via GitHub)" <gi...@apache.org> on 2026/09/25 22:16:43 UTC
[PR] fix(explore): let per-column formatting overrides return to the chart-level setting [superset]
kokhlo opened a new pull request, #44668:
URL: https://github.com/apache/superset/pull/44668
### Summary
Fixes #44667.
Root cause: `ControlForm` propagated every checkbox change as an explicit value — `onChange({ ...value, [name]: fieldValue })` — so once a key like `showCellBars` landed in `column_config` no code path could remove it. `ControlFormItem` also seeded the checkbox from the item spec's own `defaultValue`, which made an untouched column render exactly like an explicit override. The chart consumers already implement the inheritance (e.g. `TableChart.tsx` resolves `config.showCellBars === undefined ? showCellBars : config.showCellBars`; AG Grid's `useColDefs.ts` does the same), so the gap was purely in the control layer.
Fix, scoped to the props that mirror a chart-level option (`showCellBars`, `alignPositiveNegative`, `colorPositiveNegative`):
- their form-item specs are marked `resettable: true`;
- when a column carries an explicit value for such a prop, the popover shows a "Use the chart-level setting" action under the checkbox;
- the action deletes the key from `column_config` (via a new optional `onReset` channel on `ControlForm`) instead of writing another explicit value, so the column follows the chart-level option again — including when that option flips later;
- non-mirrored props (e.g. `visible`, `columnWidth`, formats) are untouched, and consumers that don't pass `onReset` see no UI change.
### Testing
- `jest src/explore/components/controls/ColumnConfigControl` → 2 suites, 7 passed (4 new tests: reset action visible only for an explicit override; reset deletes the key rather than writing a value; toggling still writes an explicit value; `resettable` set on exactly the three chart-mirrored props).
- Neighboring consumers of `ControlFormItem` (Zoom/MapView/LayerConfigs controls) → 10 passed, no regressions from the new optional prop.
- `scripts/oxfmt.sh` applied; `oxlint --config oxlint.json` clean on the touched directory.
- Verified against upstream `master` (10ca640e).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): let per-column formatting overrides return to the chart-level setting [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #44668:
URL: https://github.com/apache/superset/pull/44668#issuecomment-5840519571
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/44668?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `73.91304%` with `6 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 81.11%. Comparing base ([`10ca640`](https://app.codecov.io/gh/apache/superset/commit/10ca640e97e0b03686fe2f243f6b5f0bd331ac1c?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`6491503`](https://app.codecov.io/gh/apache/superset/commit/649150368b8f4d95e7635f5fb7b05bc6d5b79c2e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/44668?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...ntrols/ColumnConfigControl/ColumnConfigPopover.tsx](https://app.codecov.io/gh/apache/superset/pull/44668?src=pr&el=tree&filepath=superset-frontend%2Fsrc%2Fexplore%2Fcomponents%2Fcontrols%2FColumnConfigControl%2FColumnConfigPopover.tsx&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9Db2x1bW5Db25maWdDb250cm9sL0NvbHVtbkNvbmZpZ1BvcG92ZXIudHN4) | 0.00% | [4 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/44668?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...olumnConfigControl/ControlForm/ControlFormItem.tsx](https://app.codecov.io/gh/apache/superset/pull/44668?src=pr&el=tree&filepath=superset-frontend%2Fsrc%2Fexplore%2Fcomponents%2Fcontrols%2FColumnConfigControl%2FControlForm%2FControlFormItem.tsx&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9Db2x1bW5Db25maWdDb250cm9sL0NvbnRyb2xGb3JtL0NvbnRyb2xGb3JtSXRlbS50c3g=) | 90.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/44668?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...controls/ColumnConfigControl/ControlForm/index.tsx](https://app.codecov.io/gh/apache/superset/pull/44668?src=pr&el=tree&filepath=superset-frontend%2Fsrc%2Fexplore%2Fcomponents%2Fcontrols%2FColumnConfigControl%2FControlForm%2Findex.tsx&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9Db2x1bW5Db25maWdDb250cm9sL0NvbnRyb2xGb3JtL2luZGV4LnRzeA==) | 83.33% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/44668?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #44668 +/- ##
==========================================
- Coverage 81.11% 81.11% -0.01%
==========================================
Files 2955 2955
Lines 178335 178353 +18
Branches 41311 41320 +9
==========================================
+ Hits 144658 144668 +10
- Misses 30971 30972 +1
- Partials 2706 2713 +7
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/44668/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [javascript](https://app.codecov.io/gh/apache/superset/pull/44668/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `76.54% <73.91%> (+0.03%)` | :arrow_up: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/44668/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
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/44668?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): let per-column formatting overrides return to the chart-level setting [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #44668:
URL: https://github.com/apache/superset/pull/44668#discussion_r4109097058
##########
superset-frontend/src/explore/components/controls/ColumnConfigControl/ControlForm/ControlFormItem.tsx:
##########
@@ -47,16 +51,25 @@ export function ControlFormItem({
width,
validators,
onChange,
+ onReset,
value: initialValue,
defaultValue,
controlType,
+ resettable = false,
...props
}: ControlFormItemProps) {
- const { sizeUnit } = useTheme();
+ const { sizeUnit, fontSizeXS } = useTheme();
const [hovered, setHovered] = useState(false);
const [value, setValue] = useState(
initialValue === undefined ? defaultValue : initialValue,
);
+ const [prevInitialValue, setPrevInitialValue] = useState(initialValue);
+ if (initialValue !== prevInitialValue) {
+ // the parent value changed outside this item (e.g. the reset action
+ // removed the key) — follow it instead of keeping the stale local state
+ setPrevInitialValue(initialValue);
+ setValue(initialValue === undefined ? defaultValue : initialValue);
+ }
Review Comment:
**Suggestion:** After reset, the checkbox falls back to the item default instead of the actual chart-level value, so it can display the wrong effective setting.
**Assessment:** 🟠 `Major` · 🔁 `Occurrence: Sometimes` · 🏷️ `Api mismatch`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=770c82a0cdf74ae2a07edc5bbd3bf7f6&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=770c82a0cdf74ae2a07edc5bbd3bf7f6&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/explore/components/controls/ColumnConfigControl/ControlForm/ControlFormItem.tsx
**Line:** 63:72
**Comment:**
*Api Mismatch: After reset, the checkbox falls back to the item default instead of the actual chart-level value, so it can display the wrong effective setting.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44668&comment_hash=9dea070cc7f53d31059c443dcfe9b12b49cb878b37e027e994dfa4303dcf1953&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44668&comment_hash=9dea070cc7f53d31059c443dcfe9b12b49cb878b37e027e994dfa4303dcf1953&reaction=dislike'>👎</a>
##########
superset-frontend/src/explore/components/controls/ColumnConfigControl/ControlForm/ControlFormItem.tsx:
##########
@@ -94,15 +113,35 @@ export function ControlFormItem({
onMouseLeave={() => setHovered(false)}
>
{controlType === 'Checkbox' ? (
- <ControlFormItemComponents.Checkbox
- value={value as boolean}
- onChange={handleChange}
- name={name}
- label={label}
- description={description}
- validationErrors={validationErrors}
- {...props}
- />
+ <>
+ <ControlFormItemComponents.Checkbox
+ value={value as boolean}
+ onChange={handleChange}
+ name={name}
+ label={label}
+ description={description}
+ validationErrors={validationErrors}
+ {...props}
+ />
+ {hasOverride && onReset && (
+ <div
+ css={{
+ marginTop: sizeUnit,
+ paddingLeft: sizeUnit * 6,
+ }}
+ >
+ <Button
+ type="link"
+ size="small"
+ css={{ padding: 0, height: 'auto', fontSize: fontSizeXS }}
+ onClick={onReset}
Review Comment:
**Suggestion:** Clicking reset soon after changing the checkbox can let the pending debounced update run afterward and recreate the removed per-column override.
**Assessment:** 🟠 `Major` · 🔁 `Occurrence: Rarely` · 🏷️ `Race condition`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=8c6f3b7185054f0880b4f77891c04abb&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=8c6f3b7185054f0880b4f77891c04abb&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/explore/components/controls/ColumnConfigControl/ControlForm/ControlFormItem.tsx
**Line:** 137:137
**Comment:**
*Race Condition: Clicking reset soon after changing the checkbox can let the pending debounced update run afterward and recreate the removed per-column override.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44668&comment_hash=8b122021637c3ae8d82889d0fa48c2547c033fe260dc2a43f92beba14af586f5&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44668&comment_hash=8b122021637c3ae8d82889d0fa48c2547c033fe260dc2a43f92beba14af586f5&reaction=dislike'>👎</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): let per-column formatting overrides return to the chart-level setting [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #44668:
URL: https://github.com/apache/superset/pull/44668#discussion_r4109232414
##########
superset-frontend/src/explore/components/controls/ColumnConfigControl/ColumnConfigPopover.tsx:
##########
@@ -79,7 +79,17 @@ export default function ColumnConfigPopover({
key: i.toString(),
label: item.tab,
children: (
- <ControlForm onChange={onChange} value={column.config}>
+ <ControlForm
+ onChange={onChange}
+ onReset={name =>
+ onChange(
+ Object.fromEntries(
+ Object.entries(column.config).filter(([key]) => key !== name),
+ ),
+ )
+ }
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Duplicated onReset handler</b></div>
<div id="fix">
The `onReset` handler added at lines 84-90 and lines 106-112 is character-identical in both the tabbed and non-tab `ControlForm` branches. Extract it once (e.g. `handleReset`) so a future change to the reset semantics cannot diverge between the two branches.
</div>
</div>
<small><i>Code Review Run #bcf0b3</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): let per-column formatting overrides return to the chart-level setting [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #44668:
URL: https://github.com/apache/superset/pull/44668#discussion_r4409429007
##########
superset-frontend/src/explore/components/controls/ColumnConfigControl/ColumnConfigPopover.tsx:
##########
@@ -79,7 +79,17 @@ export default function ColumnConfigPopover({
key: i.toString(),
label: item.tab,
children: (
- <ControlForm onChange={onChange} value={column.config}>
+ <ControlForm
+ onChange={onChange}
+ onReset={name =>
Review Comment:
Resetting an override rebuilds the config from the committed `column.config`, so it ignores edits ControlForm is still holding in its debounce buffer. If someone changes Min width from 100 to 120 and clicks "Use the chart-level setting" within the debounce window, this emits width 100, the resulting prop update cancels the pending write, and the input can still show 120 while 100 is what gets saved (same closure again at line 106). Should the key deletion live in ControlForm, starting from its latest pending value, so unrelated pending edits survive and the two call sites share one implementation?
##########
superset-frontend/src/explore/components/controls/ColumnConfigControl/ControlForm/ControlForm.reset.test.tsx:
##########
@@ -0,0 +1,148 @@
+/**
+ * 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 { GenericDataType } from '@apache-superset/core/common';
+import { render, screen, userEvent } from 'spec/helpers/testing-library';
+import ColumnConfigPopover from 'src/explore/components/controls/ColumnConfigControl/ColumnConfigPopover';
+import ControlForm, {
+ ControlFormItem,
+ ControlFormRow,
+} from 'src/explore/components/controls/ColumnConfigControl/ControlForm';
+import { ColumnConfigFormLayout } from 'src/explore/components/controls/ColumnConfigControl/types';
+
+const CHECKBOX = (
+ <ControlFormItem
+ name="showCellBars"
+ controlType="Checkbox"
+ label="Show cell bars"
+ description="Whether to display a bar chart background in table columns"
+ defaultValue
+ debounceDelay={0}
+ resettable
+ />
+);
+
+const setup = (props = {}) => (
+ <ControlForm
+ onChange={jest.fn()}
+ onReset={jest.fn()}
+ value={{ showCellBars: false }}
+ {...props}
+ >
+ <ControlFormRow>{CHECKBOX}</ControlFormRow>
+ </ControlForm>
+);
+
+// eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
+describe('ColumnConfigControl reset to chart-level setting', () => {
+ test('shows the reset action only for an explicit per-column override', () => {
+ const { rerender } = render(setup());
+ expect(
+ screen.getByRole('button', { name: /use the chart-level setting/i }),
+ ).toBeInTheDocument();
+
+ // no explicit value in the column config: the column already follows
+ // the chart-level option, so no reset action is offered
+ rerender(setup({ value: {} }));
+ expect(
+ screen.queryByRole('button', { name: /use the chart-level setting/i }),
+ ).not.toBeInTheDocument();
+ });
+
+ test('reset deletes the key instead of writing an explicit value', async () => {
+ const onChange = jest.fn();
+ const onReset = jest.fn();
+ render(
+ <ControlForm
+ onChange={onChange}
+ onReset={onReset}
+ value={{ showCellBars: false }}
+ >
+ <ControlFormRow>{CHECKBOX}</ControlFormRow>
+ </ControlForm>,
+ );
+
+ await userEvent.click(
+ screen.getByRole('button', { name: /use the chart-level setting/i }),
+ );
+ expect(onReset).toHaveBeenCalledWith('showCellBars');
+ // the key must be gone from the propagated value, not set to true/false
+ expect(onChange).not.toHaveBeenCalledWith(
+ expect.objectContaining({ showCellBars: expect.anything() }),
+ );
+ });
+
+ test('reset through the popover removes only the reset override', async () => {
+ // ControlForm alone only proves the callback fires. The deletion happens in the
+ // popover, so exercising ControlForm with a mocked onReset stays green even if that
+ // filter never runs. Render the popover and assert on the value it emits.
+ const onChange = jest.fn();
+ render(
+ <ColumnConfigPopover
+ column={{
+ isChildColumn: false,
+ isTimeComparisonColumn: false,
+ name: 'my_column',
+ type: GenericDataType.String,
+ config: { showCellBars: false, d3NumberFormat: '.2f' },
+ }}
+ configFormLayout={
+ {
+ [GenericDataType.String]: [['showCellBars', 'd3NumberFormat']],
+ } as ColumnConfigFormLayout
+ }
+ onChange={onChange}
+ />,
+ );
+
+ await userEvent.click(
+ screen.getByRole('button', { name: /use the chart-level setting/i }),
+ );
+ // the override is gone, the unrelated formatting override survives
+ expect(onChange).toHaveBeenCalledWith({ d3NumberFormat: '.2f' });
Review Comment:
`toHaveBeenCalledWith` treats an `undefined`-valued property as equal to a missing one, so this still passes if the popover emits `{ d3NumberFormat: '.2f', showCellBars: undefined }` instead of deleting the key, and the point of the PR is that the key is gone. Could this inspect `onChange.mock.calls[0][0]` and assert `not.toHaveProperty('showCellBars')` (or use `toStrictEqual`)?
##########
superset-frontend/src/explore/components/controls/ColumnConfigControl/ControlForm/ControlForm.reset.test.tsx:
##########
@@ -0,0 +1,148 @@
+/**
+ * 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 { GenericDataType } from '@apache-superset/core/common';
+import { render, screen, userEvent } from 'spec/helpers/testing-library';
+import ColumnConfigPopover from 'src/explore/components/controls/ColumnConfigControl/ColumnConfigPopover';
+import ControlForm, {
+ ControlFormItem,
+ ControlFormRow,
+} from 'src/explore/components/controls/ColumnConfigControl/ControlForm';
+import { ColumnConfigFormLayout } from 'src/explore/components/controls/ColumnConfigControl/types';
+
+const CHECKBOX = (
+ <ControlFormItem
+ name="showCellBars"
+ controlType="Checkbox"
+ label="Show cell bars"
+ description="Whether to display a bar chart background in table columns"
+ defaultValue
+ debounceDelay={0}
+ resettable
+ />
+);
+
+const setup = (props = {}) => (
+ <ControlForm
+ onChange={jest.fn()}
+ onReset={jest.fn()}
+ value={{ showCellBars: false }}
+ {...props}
+ >
+ <ControlFormRow>{CHECKBOX}</ControlFormRow>
+ </ControlForm>
+);
+
+// eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
+describe('ColumnConfigControl reset to chart-level setting', () => {
+ test('shows the reset action only for an explicit per-column override', () => {
+ const { rerender } = render(setup());
+ expect(
+ screen.getByRole('button', { name: /use the chart-level setting/i }),
+ ).toBeInTheDocument();
+
+ // no explicit value in the column config: the column already follows
+ // the chart-level option, so no reset action is offered
+ rerender(setup({ value: {} }));
+ expect(
+ screen.queryByRole('button', { name: /use the chart-level setting/i }),
+ ).not.toBeInTheDocument();
+ });
+
+ test('reset deletes the key instead of writing an explicit value', async () => {
+ const onChange = jest.fn();
+ const onReset = jest.fn();
+ render(
+ <ControlForm
+ onChange={onChange}
+ onReset={onReset}
+ value={{ showCellBars: false }}
+ >
+ <ControlFormRow>{CHECKBOX}</ControlFormRow>
+ </ControlForm>,
+ );
+
+ await userEvent.click(
+ screen.getByRole('button', { name: /use the chart-level setting/i }),
+ );
+ expect(onReset).toHaveBeenCalledWith('showCellBars');
+ // the key must be gone from the propagated value, not set to true/false
+ expect(onChange).not.toHaveBeenCalledWith(
+ expect.objectContaining({ showCellBars: expect.anything() }),
+ );
+ });
+
+ test('reset through the popover removes only the reset override', async () => {
+ // ControlForm alone only proves the callback fires. The deletion happens in the
+ // popover, so exercising ControlForm with a mocked onReset stays green even if that
+ // filter never runs. Render the popover and assert on the value it emits.
+ const onChange = jest.fn();
+ render(
+ <ColumnConfigPopover
+ column={{
Review Comment:
`ColumnConfigInfo` requires a `key` (types.ts), and this `column` fixture omits it, so the frontend type-check will report a missing property for this test file. Should this add `key: 'my_column'`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): let per-column formatting overrides return to the chart-level setting [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #44668:
URL: https://github.com/apache/superset/pull/44668#discussion_r4134516253
##########
superset-frontend/src/explore/components/controls/ColumnConfigControl/ColumnConfigPopover.tsx:
##########
@@ -79,7 +79,17 @@ export default function ColumnConfigPopover({
key: i.toString(),
label: item.tab,
children: (
- <ControlForm onChange={onChange} value={column.config}>
+ <ControlForm
+ onChange={onChange}
+ onReset={name =>
+ onChange(
+ Object.fromEntries(
+ Object.entries(column.config).filter(([key]) => key !== name),
+ ),
+ )
+ }
Review Comment:
<!-- Bito Reply -->
Moving the key-deletion logic into `ControlForm` is a sound architectural improvement. It centralizes the reset semantics, reduces code duplication across call sites, and ensures consistent behavior, which aligns with the reviewer's goal of preventing divergence in reset logic. Since `ControlForm` already manages the `onChange` callback and the `value` state, it is well-positioned to handle the key removal internally.
**superset-frontend/src/explore/components/controls/ColumnConfigControl/ColumnConfigPopover.tsx**
```
<ControlForm
onChange={onChange}
onReset={name => {
const { [name]: _, ...rest } = column.config;
onChange(rest);
}}
/>
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): let per-column formatting overrides return to the chart-level setting [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #44668:
URL: https://github.com/apache/superset/pull/44668#issuecomment-6488262209
## <a href="https://codeant.ai/" title="CodeAnt AI"><img src="https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/codeant-block-purple-v2.svg" width="28" height="28" align="absmiddle" style="vertical-align:middle" alt="CodeAnt" /></a> CodeAnt PR Risk
**🟡 Medium Risk**
- The PR needs attention before merging.
- A pending debounced checkbox change can run after reset and recreate the per-column override.
- After reset, the checkbox falls back to its static default rather than the chart-level value, so it can display the wrong setting.
**Assessed commit:** `5a27ceeb3c6c`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): let per-column formatting overrides return to the chart-level setting [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44668:
URL: https://github.com/apache/superset/pull/44668#issuecomment-5840418128
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): let per-column formatting overrides return to the chart-level setting [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44668:
URL: https://github.com/apache/superset/pull/44668#issuecomment-5968925193
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] fix(explore): let per-column formatting overrides return to the chart-level setting [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #44668:
URL: https://github.com/apache/superset/pull/44668#discussion_r4134518557
##########
superset-frontend/src/explore/components/controls/ColumnConfigControl/ControlForm/index.tsx:
##########
@@ -115,6 +123,16 @@ export default function ControlForm({
[name]: fieldValue,
});
},
+ ...(onReset
+ ? {
+ onReset() {
+ if (onItemReset) {
+ onItemReset();
+ }
+ onReset(name);
+ },
+ }
+ : {}),
Review Comment:
Agreed—this is a real race. Reset should remain immediate, but it must cancel the pending debounce for that field before deleting the key. Routing reset through the debounced channel would delay the reset and still risks ordering issues.
The current debounce map is keyed only by `debounceDelay`, so cancelling it could also discard unrelated fields using the same delay. Make the debounced handlers field-specific:
```tsx
const debouncedOnChange = useMemo(
() =>
new Map(
Children.toArray(children)
.flatMap(row => ('children' in row.props ? Children.toArray(row.props.children) : []))
.map(item => {
const { name, debounceDelay = Constants.FAST_DEBOUNCE } = item.props;
return [
name,
debounce((fieldValue: JsonValue) => {
onChange({
...value,
[name]: fieldValue,
});
}, debounceDelay),
];
}),
),
[children, onChange, value],
);
```
Then, in the reset wrapper:
```tsx
onReset() {
debouncedOnChange.get(name)?.cancel();
onItemReset?.();
onReset(name);
}
```
The exact construction can be adapted to the existing child traversal, but the important points are:
1. Keep reset synchronous.
2. Cancel only that field’s pending write.
3. Ensure a delayed toggle cannot re-add the removed key afterward.
A regression test should toggle the checkbox, immediately reset it, advance timers, and verify that the resettable key remains absent.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): let per-column formatting overrides return to the chart-level setting [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #44668:
URL: https://github.com/apache/superset/pull/44668#issuecomment-5967394342
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 95c02392ecf5ea5ab1c542ab25934f59f104ffa2 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6ac0c2683913cd000892449f |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-44668--superset-docs-preview.netlify.app](https://deploy-preview-44668--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#DcpBCoAgEEDRu8y6yFIsu0HHsFFLMBUzKKK7N6v_F--FXNKRK8wADaxFR9zpnb87IaQcW0zhOiIlOr-1xZ62EjQ2h_QshqjUyHCQE1c9R8MYm9QghHLw_Q) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-44668--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): let per-column formatting overrides return to the chart-level setting [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #44668:
URL: https://github.com/apache/superset/pull/44668#discussion_r4134511971
##########
superset-frontend/src/explore/components/controls/ColumnConfigControl/types.ts:
##########
@@ -48,7 +48,15 @@ export interface ColumnConfigInfo {
export type ControlFormItemDefaultSpec = ControlFormItemSpec<
keyof typeof ControlFormItemComponents
->;
+> & {
+ /**
+ * Marks a checkbox whose value mirrors a chart-level option: when the
+ * per-column value is an explicit override, the popover offers a reset
+ * action that deletes the key from column_config so the column follows
+ * the chart-level setting again.
+ */
+ resettable?: boolean;
Review Comment:
`resettable` is typed generically on `ControlFormItemDefaultSpec`, so any control spec can set `resettable: true`, but only the `Checkbox` branch in `ControlFormItem` ever reads it. A resettable non-checkbox field would compile cleanly and silently render no reset action. Would it be worth scoping the type to checkbox specs, or wiring the reset UI for other control types too?
##########
superset-frontend/src/explore/components/controls/ColumnConfigControl/ControlForm/ControlForm.reset.test.tsx:
##########
@@ -0,0 +1,115 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { render, screen, userEvent } from 'spec/helpers/testing-library';
+import ControlForm, {
+ ControlFormItem,
+ ControlFormRow,
+} from 'src/explore/components/controls/ColumnConfigControl/ControlForm';
+
+const CHECKBOX = (
+ <ControlFormItem
+ name="showCellBars"
+ controlType="Checkbox"
+ label="Show cell bars"
+ description="Whether to display a bar chart background in table columns"
+ defaultValue
+ debounceDelay={0}
Review Comment:
Every test in this file passes `debounceDelay={0}`, which routes through `ControlForm`'s synchronous `0` bucket and never exercises `lodash.debounce`. The three resettable checkboxes use a real 200ms delay in production, so this suite can't catch a regression where a pending debounced write fires after a reset and re-adds the key it just removed. Could you add a case using the production 200ms delay with fake timers — toggle, reset within the window, advance the timer, and assert the key is still gone?
##########
superset-frontend/src/explore/components/controls/ColumnConfigControl/ControlForm/ControlFormItem.tsx:
##########
@@ -94,15 +113,35 @@ export function ControlFormItem({
onMouseLeave={() => setHovered(false)}
>
{controlType === 'Checkbox' ? (
- <ControlFormItemComponents.Checkbox
- value={value as boolean}
- onChange={handleChange}
- name={name}
- label={label}
- description={description}
- validationErrors={validationErrors}
- {...props}
- />
+ <>
+ <ControlFormItemComponents.Checkbox
+ value={value as boolean}
+ onChange={handleChange}
+ name={name}
+ label={label}
+ description={description}
+ validationErrors={validationErrors}
+ {...props}
+ />
+ {hasOverride && onReset && (
Review Comment:
Clicking reset removes the per-column override, which flips `hasOverride` to false and unmounts this button on the next render — a keyboard user who just activated it loses focus to the document instead of staying inside the popover. Could focus move to the checkbox (or another stable element) right after reset instead of being dropped?
##########
superset-frontend/src/explore/components/controls/ColumnConfigControl/ControlForm/ControlFormItem.tsx:
##########
@@ -94,15 +113,35 @@ export function ControlFormItem({
onMouseLeave={() => setHovered(false)}
>
{controlType === 'Checkbox' ? (
- <ControlFormItemComponents.Checkbox
- value={value as boolean}
- onChange={handleChange}
- name={name}
- label={label}
- description={description}
- validationErrors={validationErrors}
- {...props}
- />
+ <>
+ <ControlFormItemComponents.Checkbox
+ value={value as boolean}
+ onChange={handleChange}
+ name={name}
+ label={label}
+ description={description}
+ validationErrors={validationErrors}
+ {...props}
+ />
+ {hasOverride && onReset && (
+ <div
+ css={{
+ marginTop: sizeUnit,
+ paddingLeft: sizeUnit * 6,
+ }}
+ >
+ <Button
+ type="link"
+ size="small"
+ css={{ padding: 0, height: 'auto', fontSize: fontSizeXS }}
+ onClick={onReset}
+ >
+ <Icons.RollbackOutlined iconSize="s" />{' '}
Review Comment:
`Icons.RollbackOutlined` carries an implicit, untranslated label, so a screen reader may announce something like "rollback Use the chart-level setting" instead of just the translated action text. Could the icon be marked decorative (e.g. `aria-hidden`) so the button's text alone supplies its accessible name?
##########
superset-frontend/src/explore/components/controls/ColumnConfigControl/ControlForm/index.tsx:
##########
@@ -115,6 +123,16 @@ export default function ControlForm({
[name]: fieldValue,
});
},
+ ...(onReset
+ ? {
+ onReset() {
+ if (onItemReset) {
+ onItemReset();
+ }
+ onReset(name);
+ },
+ }
+ : {}),
Review Comment:
Agreed—confirmed by reproducing it: toggle a resettable checkbox (starting the ~200ms debounce), then click "Use the chart-level setting" before it fires. The reset removes the key immediately, but the already-scheduled write still lands afterward with the stale pre-reset value and silently re-adds the key. Should the reset path cancel or flush any pending debounced write for that field first, or route resets through the same debounced channel as toggles?
##########
superset-frontend/src/explore/components/controls/ColumnConfigControl/ControlForm/ControlFormItem.tsx:
##########
@@ -47,16 +51,25 @@ export function ControlFormItem({
width,
validators,
onChange,
+ onReset,
value: initialValue,
defaultValue,
controlType,
+ resettable = false,
...props
}: ControlFormItemProps) {
- const { sizeUnit } = useTheme();
+ const { sizeUnit, fontSizeXS } = useTheme();
const [hovered, setHovered] = useState(false);
const [value, setValue] = useState(
initialValue === undefined ? defaultValue : initialValue,
);
+ const [prevInitialValue, setPrevInitialValue] = useState(initialValue);
+ if (initialValue !== prevInitialValue) {
+ // the parent value changed outside this item (e.g. the reset action
+ // removed the key) — follow it instead of keeping the stale local state
+ setPrevInitialValue(initialValue);
+ setValue(initialValue === undefined ? defaultValue : initialValue);
+ }
Review Comment:
Agreed—after a reset, the checkbox's local value falls back to the item spec's static `defaultValue` (e.g. `true` for showCellBars), not the actual current chart-level setting, since that value is never passed into this component. If the chart-level option differs from the spec default, the checkbox can show the wrong effective state right after reset. Could the popover pass the resolved chart-level value down as the fallback instead of the static per-field default?
##########
superset-frontend/src/explore/components/controls/ColumnConfigControl/ColumnConfigPopover.tsx:
##########
@@ -79,7 +79,17 @@ export default function ColumnConfigPopover({
key: i.toString(),
label: item.tab,
children: (
- <ControlForm onChange={onChange} value={column.config}>
+ <ControlForm
+ onChange={onChange}
+ onReset={name =>
+ onChange(
+ Object.fromEntries(
+ Object.entries(column.config).filter(([key]) => key !== name),
+ ),
+ )
+ }
Review Comment:
Agreed—both `ControlForm` call sites in this file implement the exact same key-deletion closure. Since `ControlForm` already owns the value and calls `onChange` with the full object on every change, would it make more sense for the `onReset` channel to delete the key internally and call `onChange` itself, rather than requiring every caller to reimplement the same filter?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): let per-column formatting overrides return to the chart-level setting [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #44668:
URL: https://github.com/apache/superset/pull/44668#discussion_r4172485741
##########
superset-frontend/src/explore/components/controls/ColumnConfigControl/ControlForm/ControlForm.reset.test.tsx:
##########
@@ -0,0 +1,115 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { render, screen, userEvent } from 'spec/helpers/testing-library';
+import ControlForm, {
+ ControlFormItem,
+ ControlFormRow,
+} from 'src/explore/components/controls/ColumnConfigControl/ControlForm';
+
+const CHECKBOX = (
+ <ControlFormItem
+ name="showCellBars"
+ controlType="Checkbox"
+ label="Show cell bars"
+ description="Whether to display a bar chart background in table columns"
+ defaultValue
+ debounceDelay={0}
+ resettable
+ />
+);
+
+const setup = (props = {}) => (
+ <ControlForm
+ onChange={jest.fn()}
+ onReset={jest.fn()}
+ value={{ showCellBars: false }}
+ {...props}
+ >
+ <ControlFormRow>{CHECKBOX}</ControlFormRow>
+ </ControlForm>
+);
+
+// eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
+describe('ColumnConfigControl reset to chart-level setting', () => {
+ test('shows the reset action only for an explicit per-column override', () => {
+ const { rerender } = render(setup());
+ expect(
+ screen.getByRole('button', { name: /use the chart-level setting/i }),
+ ).toBeInTheDocument();
+
+ // no explicit value in the column config: the column already follows
+ // the chart-level option, so no reset action is offered
+ rerender(setup({ value: {} }));
+ expect(
+ screen.queryByRole('button', { name: /use the chart-level setting/i }),
+ ).not.toBeInTheDocument();
+ });
+
+ test('reset deletes the key instead of writing an explicit value', async () => {
+ const onChange = jest.fn();
+ const onReset = jest.fn();
+ render(
+ <ControlForm
+ onChange={onChange}
+ onReset={onReset}
+ value={{ showCellBars: false }}
+ >
+ <ControlFormRow>{CHECKBOX}</ControlFormRow>
+ </ControlForm>,
+ );
+
+ await userEvent.click(
+ screen.getByRole('button', { name: /use the chart-level setting/i }),
+ );
+ expect(onReset).toHaveBeenCalledWith('showCellBars');
+ // the key must be gone from the propagated value, not set to true/false
Review Comment:
This deletion test passes even if the popover never removes the override: `onReset` is a no-op mock and `onChange` is never called. Could it exercise `ColumnConfigPopover` with `{ showCellBars: false, d3NumberFormat: ".2f" }` and assert that resetting emits `{ d3NumberFormat: ".2f" }`, preserving the unrelated setting while removing the override?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): let per-column formatting overrides return to the chart-level setting [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #44668:
URL: https://github.com/apache/superset/pull/44668#discussion_r4109095861
##########
superset-frontend/src/explore/components/controls/ColumnConfigControl/ControlForm/index.tsx:
##########
@@ -115,6 +123,16 @@ export default function ControlForm({
[name]: fieldValue,
});
},
+ ...(onReset
+ ? {
+ onReset() {
+ if (onItemReset) {
+ onItemReset();
+ }
+ onReset(name);
+ },
+ }
+ : {}),
Review Comment:
**Suggestion:** A pending debounced field change can run after reset and re-add the removed key from the stale form value, undoing the reset.
**Assessment:** 🟠 `Major` · 🔁 `Occurrence: Sometimes` · 🏷️ `Race condition`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=984d7915ab17455abe3dc12dbdcb02cf&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=984d7915ab17455abe3dc12dbdcb02cf&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/explore/components/controls/ColumnConfigControl/ControlForm/index.tsx
**Line:** 126:135
**Comment:**
*Race Condition: A pending debounced field change can run after reset and re-add the removed key from the stale form value, undoing the reset.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44668&comment_hash=7e8767c12c3ed956cc8c0794b1e8c3b60c95b1442fb6b85d9a6f64b28d8cc9dd&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44668&comment_hash=7e8767c12c3ed956cc8c0794b1e8c3b60c95b1442fb6b85d9a6f64b28d8cc9dd&reaction=dislike'>👎</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): let per-column formatting overrides return to the chart-level setting [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44668:
URL: https://github.com/apache/superset/pull/44668#issuecomment-5840461052
<!-- Bito Reply -->
The flagged issue is correct. Because `ControlFormItem` uses a debounced `onChange` handler, a pending update can trigger after the `onReset` action has already updated the parent state, causing the stale value to be written back into the configuration.
To resolve this, you should cancel any pending debounced updates when `onReset` is triggered. In `ControlFormItem.tsx`, you can use a `useRef` to track the debounced function and call its `cancel` method (if using `lodash.debounce`) inside the `onReset` handler.
Would you like me to fetch all other comments on this PR to validate and implement fixes for them as well?
**superset-frontend/src/explore/components/controls/ColumnConfigControl/ControlForm/ControlFormItem.tsx**
```
const debouncedOnChange = useMemo(
() => debounce(onChange, debounceDelay),
[onChange, debounceDelay],
);
const handleReset = () => {
debouncedOnChange.cancel();
if (onReset) onReset();
};
```
--
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