bito-code-review[bot] commented on code in PR #35416:
URL: https://github.com/apache/superset/pull/35416#discussion_r2394003633
##########
superset-frontend/src/explore/components/ExploreAlert.tsx:
##########
@@ -55,14 +62,16 @@ export const ExploreAlert = forwardRef(
showIcon
>
{primaryButtonText && primaryButtonAction && (
- <div>
+ <ButtonContainer>
{secondaryButtonAction && secondaryButtonText && (
- <Button onClick={secondaryButtonAction}>
+ <Button buttonStyle="secondary" onClick={secondaryButtonAction}>
{secondaryButtonText}
</Button>
)}
- <Button onClick={primaryButtonAction}>{primaryButtonText}</Button>
- </div>
+ <Button buttonStyle="secondary" onClick={primaryButtonAction}>
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Button styling hierarchy issue</b></div>
<div id="fix">
Both the primary and secondary buttons are using `buttonStyle="secondary"`,
which breaks the visual hierarchy and makes it impossible for users to
distinguish the primary action from the secondary action. This affects
downstream components like `ExploreChartPanel/index.tsx` and
`ControlPanelsContainer.tsx` that rely on clear visual distinction between
primary and secondary actions. Change the primary button to use the default
primary style by removing the `buttonStyle="secondary"` prop.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```suggestion
<Button onClick={primaryButtonAction}>
```
</div>
</details>
</div>
<small><i>Code Review Run <a
href=https://github.com/apache/superset/pull/35416#issuecomment-3355508081>#454879</a></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
##########
superset-frontend/src/explore/components/ExploreAlert.test.tsx:
##########
@@ -0,0 +1,248 @@
+/**
+ * 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 } from 'spec/helpers/testing-library';
+import userEvent from '@testing-library/user-event';
+import { ExploreAlert } from './ExploreAlert';
+
+test('renders with title and body text', () => {
+ render(
+ <ExploreAlert title="Test Title" bodyText="Test body text" type="info" />,
+ );
+
+ expect(screen.getByText('Test Title')).toBeInTheDocument();
+ expect(screen.getByText('Test body text')).toBeInTheDocument();
+});
+
+test('renders info type alert', () => {
+ const { container } = render(
+ <ExploreAlert title="Info Alert" bodyText="Info message" type="info" />,
+ );
+
+ expect(container.querySelector('.ant-alert-info')).toBeInTheDocument();
+});
+
+test('renders warning type alert', () => {
+ const { container } = render(
+ <ExploreAlert
+ title="Warning Alert"
+ bodyText="Warning message"
+ type="warning"
+ />,
+ );
+
+ expect(container.querySelector('.ant-alert-warning')).toBeInTheDocument();
+});
+
+test('renders error type alert', () => {
+ const { container } = render(
+ <ExploreAlert title="Error Alert" bodyText="Error message" type="error" />,
+ );
+
+ expect(container.querySelector('.ant-alert-error')).toBeInTheDocument();
+});
+
+test('renders primary button when text and action provided', () => {
+ const primaryAction = jest.fn();
+
+ render(
+ <ExploreAlert
+ title="Alert with button"
+ bodyText="Body text"
+ type="info"
+ primaryButtonText="Primary Action"
+ primaryButtonAction={primaryAction}
+ />,
+ );
+
+ expect(screen.getByText('Primary Action')).toBeInTheDocument();
+});
+
+test('calls primary button action when clicked', async () => {
+ const primaryAction = jest.fn();
+
+ render(
+ <ExploreAlert
+ title="Alert with button"
+ bodyText="Body text"
+ type="info"
+ primaryButtonText="Click Me"
+ primaryButtonAction={primaryAction}
+ />,
+ );
+
+ await userEvent.click(screen.getByText('Click Me'));
+ expect(primaryAction).toHaveBeenCalledTimes(1);
+});
+
+test('renders both primary and secondary buttons when provided', () => {
+ const primaryAction = jest.fn();
+ const secondaryAction = jest.fn();
+
+ render(
+ <ExploreAlert
+ title="Alert with buttons"
+ bodyText="Body text"
+ type="info"
+ primaryButtonText="Primary"
+ primaryButtonAction={primaryAction}
+ secondaryButtonText="Secondary"
+ secondaryButtonAction={secondaryAction}
+ />,
+ );
+
+ expect(screen.getByText('Primary')).toBeInTheDocument();
+ expect(screen.getByText('Secondary')).toBeInTheDocument();
+});
+
+test('calls secondary button action when clicked', async () => {
+ const primaryAction = jest.fn();
+ const secondaryAction = jest.fn();
+
+ render(
+ <ExploreAlert
+ title="Alert with buttons"
+ bodyText="Body text"
+ type="info"
+ primaryButtonText="Primary"
+ primaryButtonAction={primaryAction}
+ secondaryButtonText="Secondary"
+ secondaryButtonAction={secondaryAction}
+ />,
+ );
+
+ await userEvent.click(screen.getByText('Secondary'));
+ expect(secondaryAction).toHaveBeenCalledTimes(1);
+ expect(primaryAction).not.toHaveBeenCalled();
+});
+
+test('does not render buttons when only text is provided without action', ()
=> {
+ render(
+ <ExploreAlert
+ title="Alert without action"
+ bodyText="Body text"
+ type="info"
+ primaryButtonText="Primary"
+ />,
+ );
+
+ expect(screen.queryByText('Primary')).not.toBeInTheDocument();
+});
+
+test('does not render buttons when only action is provided without text', ()
=> {
+ const primaryAction = jest.fn();
+
+ render(
+ <ExploreAlert
+ title="Alert without text"
+ bodyText="Body text"
+ type="info"
+ primaryButtonAction={primaryAction}
+ />,
+ );
+
+ expect(screen.queryByRole('button')).not.toBeInTheDocument();
+});
+
+test('does not render secondary button when secondary action is missing', ()
=> {
+ const primaryAction = jest.fn();
+
+ render(
+ <ExploreAlert
+ title="Alert"
+ bodyText="Body text"
+ type="info"
+ primaryButtonText="Primary"
+ primaryButtonAction={primaryAction}
+ secondaryButtonText="Secondary"
+ />,
+ );
+
+ expect(screen.getByText('Primary')).toBeInTheDocument();
+ expect(screen.queryByText('Secondary')).not.toBeInTheDocument();
+});
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Test assertion error</b></div>
<div id="fix">
The test assertion at line 176 is testing the wrong behavior. The test 'does
not render secondary button when secondary action is missing' incorrectly
asserts that the primary button should be present, but this test should only
verify that the secondary button is not rendered. The primary button rendering
is already tested in other tests. This assertion could cause false test
failures when the component correctly renders the primary button but the test
is focused on secondary button behavior.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```suggestion
expect(screen.queryByText('Secondary')).not.toBeInTheDocument();
});
```
</div>
</details>
</div>
<small><i>Code Review Run <a
href=https://github.com/apache/superset/pull/35416#issuecomment-3355508081>#454879</a></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
##########
superset-frontend/src/components/ErrorMessage/ErrorAlert.tsx:
##########
@@ -100,6 +100,7 @@ export const ErrorAlert: React.FC<ErrorAlertProps> = ({
</span>
</div>
)}
+ {children}
</div>
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Breaking children rendering behavior</b></div>
<div id="fix">
Moving `{children}` from inside the Modal (line 132) to inside
`renderDescription` (line 103) will cause children content to appear in both
compact mode (inside the modal) and non-compact mode (in the regular alert).
This breaks the existing behavior where children were only shown in modal mode.
The change affects `renderAlert` which is used in both compact and non-compact
modes, potentially causing UI duplication and layout issues in non-compact
alerts that weren't designed to display children content.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```suggestion
</div>
)}
{compact && children}
</div>
```
</div>
</details>
</div>
<small><i>Code Review Run <a
href=https://github.com/apache/superset/pull/35416#issuecomment-3355508081>#454879</a></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: [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]
