You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "msyavuz (via GitHub)" <gi...@apache.org> on 2025/01/13 09:10:18 UTC
[PR] chore(Network Errors): Update network errors on filter bars and charts [superset]
msyavuz opened a new pull request, #31811:
URL: https://github.com/apache/superset/pull/31811
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
Update network errors on filter bars and charts specified in #31342
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
Filter Bar After:
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
Run the testing suite and check for visual changes.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [x] Has associated issue: Fixes #31342
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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] chore(Network Errors): Update network errors on filter bars and charts [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #31811:
URL: https://github.com/apache/superset/pull/31811#discussion_r1929083853
##########
superset-frontend/src/components/ErrorMessage/FrontendNetworkErrorMessage.tsx:
##########
@@ -24,10 +24,16 @@ import ErrorAlert from './ErrorAlert';
function FrontendNetworkErrorMessage({
error,
subtitle,
+ compact,
}: ErrorMessageComponentProps) {
const { level, message } = error;
return (
- <ErrorAlert errorType={t('Network Error')} message={message} type={level} />
+ <ErrorAlert
+ compact={compact}
+ errorType={t('Network Error')}
+ message={message}
+ type={level}
+ />
);
}
Review Comment:
### Unused subtitle prop <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The 'subtitle' prop is declared but never used in the component, which contradicts the expected functionality of displaying a subtitle when provided.
###### Why this matters
If a consumer passes a subtitle prop, they would expect it to be displayed, leading to confusing behavior when the subtitle doesn't appear in the UI.
###### Suggested change β *Feature Preview*
Either remove the unused subtitle prop or implement it in the ErrorAlert component:
```typescript
function FrontendNetworkErrorMessage({
error,
subtitle,
compact,
}: ErrorMessageComponentProps) {
const { level, message } = error;
return (
<ErrorAlert
compact={compact}
errorType={t('Network Error')}
message={message}
subtitle={subtitle}
type={level}
/>
);
}
```
</details>
<sub>π‘ Does this comment miss the mark? [Tell us why](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/acab52cc-66c8-47f8-ad63-6734316db410?suggestedFixEnabled=true) and Korbit will adapt to your teamβs feedback.
π¬ Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:5407c872-4161-48a9-b518-9315016dea7c -->
--
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] chore(Network Errors): Update network errors on filter bars and charts [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #31811:
URL: https://github.com/apache/superset/pull/31811#discussion_r1913570147
##########
superset-frontend/src/components/ErrorMessage/NetworkErrorMessage.tsx:
##########
@@ -0,0 +1,43 @@
+/**
+ * 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 { t } from '@superset-ui/core';
+
+import { ErrorMessageComponentProps } from './types';
+import ErrorAlert from './ErrorAlert';
+
+function NetworkErrorMessage({
+ error,
+ source = 'dashboard',
+ subtitle,
+}: ErrorMessageComponentProps) {
+ const { level, message } = error;
Review Comment:
### Unsafe error object destructuring <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Direct destructuring of error object properties without checking if they exist could lead to runtime errors.
###### Why this matters
If the error object is malformed or missing expected properties, the application could crash when trying to access undefined properties.
###### Suggested change β *Feature Preview*
Add property existence checks or provide default values during destructuring:
const { level = 'error', message = 'Unknown network error' } = error || {};
</details>
###### Chat with Korbit by mentioning @korbit-ai, and give a π or π to help Korbit improve your reviews.
<!--- korbi internal id:f100da12-bdcb-4eb1-b4a8-84ecf42ab824 -->
--
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] chore(Network Errors): Update network errors on filter bars and charts [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on code in PR #31811:
URL: https://github.com/apache/superset/pull/31811#discussion_r1932521084
##########
superset-frontend/src/components/ErrorMessage/FrontendNetworkErrorMessage.test.tsx:
##########
@@ -0,0 +1,71 @@
+/**
+ * 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 { ErrorLevel, ErrorSource, ErrorTypeEnum } from '@superset-ui/core';
+import userEvent from '@testing-library/user-event';
+import { render, screen } from 'spec/helpers/testing-library';
+import FrontendNetworkErrorMessage from './FrontendNetworkErrorMessage';
+
+jest.mock(
+ 'src/components/Icons/Icon',
+ () =>
+ ({ fileName }: { fileName: string }) => (
+ <span role="img" aria-label={fileName.replace('_', '-')} />
+ ),
+);
+
+const mockedProps = {
+ error: {
+ error_type: ErrorTypeEnum.FRONTEND_NETWORK_ERROR,
+ extra: {},
+ level: 'error' as ErrorLevel,
+ message: 'Error message',
+ },
+ source: 'dashboard' as ErrorSource,
+ subtitle: 'Error message',
+};
+
+test('should render', () => {
+ const nullExtraProps = {
+ ...mockedProps,
+ error: {
+ ...mockedProps.error,
+ extra: null,
+ },
+ };
+ const { container } = render(
+ <FrontendNetworkErrorMessage {...nullExtraProps} />,
+ );
+ expect(container).toBeInTheDocument();
+});
+
+test('should render the error message', () => {
+ render(<FrontendNetworkErrorMessage {...mockedProps} />, { useRedux: true });
+ // userEvent.click(screen.getByRole('button'));
Review Comment:
```suggestion
```
--
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] chore(Network Errors): Update network errors on filter bars and charts [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #31811:
URL: https://github.com/apache/superset/pull/31811#issuecomment-2591549791
Hey I don't want to derail here but somehow as part of my extremely large "theming PR" I somehow took on completely refactoring `Alert` and `ErrorAlert` into something much simpler, vanilla antd, that includes a `compact` prop for this type of stuff.
https://github.com/apache/superset/pull/31590/files#diff-0e958cf2ea1bc7da177b24b9cfe5aef75308a080f22e2a0cada374ade11ba2cfR121-R140
I'm working at breaking down the PR into smaller chunks, but wondering whether it may make sense to align here. Somehow there are a lot of different ErrorAlert derivatives or components that should in theory BE and ErrorAlert but don't compose things as expected.
Wondering if it'd be reasonable to bring in the Alert/ErrorAlert parts of my PR in here, though it may be a bit of a surgery.
Clearly on a merge conflict collision here.
--
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] chore(Network Errors): Update network errors on filter bars and charts [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #31811:
URL: https://github.com/apache/superset/pull/31811#issuecomment-2617092657
Curious to see how it looks when integrating the `compact` mode once you can share screenshots, hopefully looks good out-of-the-box. I don't think it's used elsewhere in the codebase, so we could alter the compact mode to make sure it looks right in this context (and hopefully other places eventually where we need compact alerts).
--
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] chore(Network Errors): Update network errors on filter bars and charts [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on PR #31811:
URL: https://github.com/apache/superset/pull/31811#issuecomment-2613052446
Sorry for the pings people, was trying to solve merge conflicts.
--
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] chore(Network Errors): Update network errors on filter bars and charts [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on PR #31811:
URL: https://github.com/apache/superset/pull/31811#issuecomment-2586557497
Based on your review schedule, I'll hold off on reviewing this PR until it's marked as ready for review. If you'd like me to take a look now, comment `/korbit-review`.
> Your admin can change your review schedule in the [Korbit Console](https://app.korbit.ai/settings)
--
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] chore(Network Errors): Update network errors on filter bars and charts [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #31811:
URL: https://github.com/apache/superset/pull/31811#issuecomment-2613216635
Merged my Alert-related PRs, and wondering if it make sense to use the `compact` mode that's now exposed
--
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] chore(Network Errors): Update network errors on filter bars and charts [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido merged PR #31811:
URL: https://github.com/apache/superset/pull/31811
--
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] chore(Network Errors): Update network errors on filter bars and charts [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #31811:
URL: https://github.com/apache/superset/pull/31811#issuecomment-2620253112
wait don't before/after look the same in the screenshots?
--
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] chore(Network Errors): Update network errors on filter bars and charts [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on PR #31811:
URL: https://github.com/apache/superset/pull/31811#issuecomment-2618938561
The screenshots show the compact mode on filter bar errors. Chart network errors didn't seem like a good fit for the compact mode.
--
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] chore(Network Errors): Update network errors on filter bars and charts [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on PR #31811:
URL: https://github.com/apache/superset/pull/31811#issuecomment-2593128412
@mistercrunch We can hold off on this pr until #31858 is merged. I can rebase this onto your changes after.
--
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