codeant-ai-for-open-source[bot] commented on code in PR #35897:
URL: https://github.com/apache/superset/pull/35897#discussion_r2590550015
##########
superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.test.tsx:
##########
@@ -180,9 +181,10 @@ test('displays the toAllRow and toTextColor flags based on
the selected numeric
expect(screen.getByText('To entire row')).toBeInTheDocument();
expect(screen.getByText('To text color')).toBeInTheDocument();
+ expect(screen.getByText('To cell bar')).toBeInTheDocument();
Review Comment:
**Suggestion:** The test uses an exact string match with `getByText('To cell
bar')`; exact-case text matches are brittle (localization/capitalization
changes) and can cause flaky failures — use a case-insensitive regex to make
the assertion robust. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
expect(screen.getByText(/to cell bar/i)).toBeInTheDocument();
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Using a case-insensitive regex makes the test less brittle to minor
capitalization/localization changes and reduces flakiness. It's a small, safe
improvement that doesn't change behavior and increases resilience of the test
assertion.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.test.tsx
**Line:** 184:184
**Comment:**
*Possible Bug: The test uses an exact string match with `getByText('To
cell bar')`; exact-case text matches are brittle (localization/capitalization
changes) and can cause flaky failures — use a case-insensitive regex to make
the assertion robust.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.tsx:
##########
@@ -466,8 +484,15 @@ export const FormattingPopoverContent = ({
initialValue={toTextColor}
>
<Checkbox
- onChange={event => setToTextColor(event.target.checked)}
+ onChange={event => {
+ const checked = event.target.checked;
+ setToTextColor(checked);
+ if (checked) {
+ setToCellBar(false);
+ }
+ }}
checked={toTextColor}
Review Comment:
**Suggestion:** The `toTextColor` Checkbox handler updates only local state
and leaves the Form's field unsynced while the component also passes `checked`,
causing the form to submit stale values; call `form.setFieldsValue` inside the
onChange handler and remove the direct `checked` prop so Form state matches UI
state. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
form.setFieldsValue({ toTextColor: checked, toCellBar:
false });
} else {
form.setFieldsValue({ toTextColor: checked });
}
}}
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Same issue as the previous checkbox: the UI state is controlled locally but
the Form field (FormItem with name) isn't updated, leading to inconsistent
submitted data. Updating the Form via `form.setFieldsValue` or delegating
control to Form fixes this.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.tsx
**Line:** 492:494
**Comment:**
*Possible Bug: The `toTextColor` Checkbox handler updates only local
state and leaves the Form's field unsynced while the component also passes
`checked`, causing the form to submit stale values; call `form.setFieldsValue`
inside the onChange handler and remove the direct `checked` prop so Form state
matches UI state.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.tsx:
##########
@@ -447,8 +458,15 @@ export const FormattingPopoverContent = ({
initialValue={toAllRow}
>
<Checkbox
- onChange={event => setToAllRow(event.target.checked)}
+ onChange={event => {
+ const checked = event.target.checked;
+ setToAllRow(checked);
+ if (checked) {
+ setToCellBar(false);
+ }
+ }}
checked={toAllRow}
Review Comment:
**Suggestion:** The `toAllRow` Checkbox handler updates only local React
state but does not update the Form's field value, while the Checkbox also
passes a `checked` prop (overriding Form control). This desynchronizes Form
values and component state so submitting the form may not include the user's
updated checkbox state; update the form via `form.setFieldsValue` and remove
the direct `checked` prop to keep Form state authoritative. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
form.setFieldsValue({ toAllRow: checked, toCellBar:
false });
} else {
form.setFieldsValue({ toAllRow: checked });
}
}}
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The FormItem is controlling the field via name/valuePropName, but the
Checkbox also receives a controlled `checked` prop driven by local state. The
form value therefore can diverge from what will be submitted. Calling
`form.setFieldsValue` (or removing the `checked` prop and letting Form control
the input) resolves a real bug.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.tsx
**Line:** 466:468
**Comment:**
*Possible Bug: The `toAllRow` Checkbox handler updates only local React
state but does not update the Form's field value, while the Checkbox also
passes a `checked` prop (overriding Form control). This desynchronizes Form
values and component state so submitting the form may not include the user's
updated checkbox state; update the form via `form.setFieldsValue` and remove
the direct `checked` prop to keep Form state authoritative.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.test.tsx:
##########
@@ -193,15 +195,17 @@ test('displays the toAllRow and toTextColor flags based
on the selected string t
expect(screen.getByText('To entire row')).toBeInTheDocument();
expect(screen.getByText('To text color')).toBeInTheDocument();
+ expect(screen.queryByText('To cell bar')).not.toBeInTheDocument();
Review Comment:
**Suggestion:** The negative assertions use an exact string match via
`queryByText('To cell bar')`, which is brittle; switch to a case-insensitive
regex to avoid false negatives due to capitalization or minor text changes.
[possible bug]
**Severity Level:** Critical 🚨
```suggestion
expect(screen.queryByText(/to cell bar/i)).not.toBeInTheDocument();
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Same rationale as the previous suggestion: switching to a case-insensitive
regex for negative assertions reduces the chance of false negatives due to
trivial text changes. It's a pragmatic, low-risk improvement to test robustness.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.test.tsx
**Line:** 198:198
**Comment:**
*Possible Bug: The negative assertions use an exact string match via
`queryByText('To cell bar')`, which is brittle; switch to a case-insensitive
regex to avoid false negatives due to capitalization or minor text changes.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.tsx:
##########
@@ -476,6 +501,33 @@ export const FormattingPopoverContent = ({
</Col>
</Row>
)}
+ {showOperatorFields && columnTypeNumeric && showToCellBar && (
+ <Row gutter={20}>
+ <Col span={1}>
+ <FormItem
+ name="toCellBar"
+ valuePropName="checked"
+ initialValue={toCellBar}
+ >
+ <Checkbox
+ onChange={event => {
+ const checked = event.target.checked;
+ setToCellBar(checked);
+ if (checked) {
+ setToTextColor(false);
+ setToAllRow(false);
+ }
+ }}
+ checked={toCellBar}
Review Comment:
**Suggestion:** The `toCellBar` Checkbox onChange updates only component
state and doesn't sync the Form fields; because the Checkbox also receives a
`checked` prop, the Form's values will remain stale on submit — call
`form.setFieldsValue` to update relevant form fields (and remove the direct
`checked` prop). [possible bug]
**Severity Level:** Critical 🚨
```suggestion
form.setFieldsValue({
toCellBar: checked,
toTextColor: false,
toAllRow: false,
});
} else {
form.setFieldsValue({ toCellBar: checked });
}
}}
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The component updates only local state while the FormItem owns the actual
form field; keeping the Checkbox `checked` tied to local state causes the form
to submit stale values. Propagating changes to the Form (or removing the local
controlled prop) fixes the bug.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.tsx
**Line:** 519:521
**Comment:**
*Possible Bug: The `toCellBar` Checkbox onChange updates only component
state and doesn't sync the Form fields; because the Checkbox also receives a
`checked` prop, the Form's values will remain stale on submit — call
`form.setFieldsValue` to update relevant form fields (and remove the direct
`checked` prop).
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
