korbit-ai[bot] commented on code in PR #34674: URL: https://github.com/apache/superset/pull/34674#discussion_r2273715365
########## superset-frontend/src/components/ErrorMessage/CustomDocLink.tsx: ########## @@ -0,0 +1,35 @@ +/** + * 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 { useTheme } from '@superset-ui/core'; +import { Icons } from '@superset-ui/core/components'; + +export type CustomDocLinkProps = { + url: string; + label: string; +}; + +export const CustomDocLink = ({ url, label }: CustomDocLinkProps) => { + const theme = useTheme(); + return ( + <a href={url} target="_blank" rel="noopener noreferrer"> + {label} <Icons.Full iconSize="m" iconColor={theme.colorPrimary} /> Review Comment: ### Unclear Icon Purpose for Documentation Link <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The Icons.Full component usage is ambiguous as it's not clear what icon it represents in the context of a documentation link. ###### Why this matters Using a generic 'Full' icon may confuse users about where the link leads to. For documentation links, a more specific and commonly recognized icon like 'ExternalLink' would better indicate the link's behavior. ###### Suggested change ∙ *Feature Preview* Replace the generic Full icon with a more appropriate external link icon: ```typescript <Icons.ExternalLink iconSize="m" iconColor={theme.colorPrimary} /> ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/85f50d34-bcd0-4de0-ab86-1d401ba26b64/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/85f50d34-bcd0-4de0-ab86-1d401ba26b64?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/85f50d34-bcd0-4de0-ab86-1d401ba26b64?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/85f50d34-bcd0-4de0-ab86-1d401ba26b64?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/85f50d34-bcd0-4de0-ab86-1d401ba26b64) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:10358e01-6c24-4f6b-9257-8a634606617d --> [](10358e01-6c24-4f6b-9257-8a634606617d) ########## superset-frontend/src/components/ErrorMessage/CustomDocLink.tsx: ########## @@ -0,0 +1,35 @@ +/** + * 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 { useTheme } from '@superset-ui/core'; +import { Icons } from '@superset-ui/core/components'; + +export type CustomDocLinkProps = { + url: string; + label: string; +}; + +export const CustomDocLink = ({ url, label }: CustomDocLinkProps) => { + const theme = useTheme(); + return ( + <a href={url} target="_blank" rel="noopener noreferrer"> Review Comment: ### Missing URL Validation <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The component doesn't validate the URL parameter, which could lead to security issues or broken links. ###### Why this matters Invalid or malicious URLs could cause the application to behave unexpectedly or create security vulnerabilities. ###### Suggested change ∙ *Feature Preview* Add URL validation before rendering: ```typescript export const CustomDocLink = ({ url, label }: CustomDocLinkProps) => { const theme = useTheme(); const isValidUrl = (urlString: string) => { try { new URL(urlString); return true; } catch { return false; } }; if (!isValidUrl(url)) { return null; } return ( <a href={url} target="_blank" rel="noopener noreferrer"> {label} <Icons.ExternalLink iconSize="m" iconColor={theme.colorPrimary} /> </a> ); }; ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f5faf13f-9b76-46a8-b848-70c92c7a7491/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f5faf13f-9b76-46a8-b848-70c92c7a7491?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f5faf13f-9b76-46a8-b848-70c92c7a7491?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f5faf13f-9b76-46a8-b848-70c92c7a7491?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f5faf13f-9b76-46a8-b848-70c92c7a7491) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:af5dfa3f-a941-43c0-b0b8-25cd68805b7b --> [](af5dfa3f-a941-43c0-b0b8-25cd68805b7b) ########## superset/db_engine_specs/base.py: ########## @@ -1329,9 +1329,13 @@ def extract_errors( cls, ex: Exception, context: dict[str, Any] | None = None ) -> list[SupersetError]: raw_message = cls._extract_error_message(ex) + config_custom_errors = app.config.get("CUSTOM_DATABASE_ERRORS", {}) context = context or {} - for regex, (message, error_type, extra) in cls.custom_errors.items(): + for regex, (message, error_type, extra) in [ + *config_custom_errors.items(), + *cls.custom_errors.items(), + ]: if match := regex.search(raw_message): Review Comment: ### Sequential Regex Pattern Matching <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? Multiple regex patterns are evaluated sequentially until a match is found, which can be inefficient for large numbers of patterns. ###### Why this matters Linear search through regex patterns can become a performance bottleneck when there are many patterns to check or when the function is called frequently. ###### Suggested change ∙ *Feature Preview* Consider compiling patterns into a single regex using alternation if possible, or using a more efficient pattern matching strategy like a trie for literal matches: ```python import re combined_pattern = re.compile('|'.join(f'(?P<{i}>{pattern.pattern})' for i, pattern in enumerate(patterns))) if match := combined_pattern.search(raw_message): matched_group = match.lastgroup ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f0d86481-3f52-4157-98dc-ceef89b916f3/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f0d86481-3f52-4157-98dc-ceef89b916f3?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f0d86481-3f52-4157-98dc-ceef89b916f3?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f0d86481-3f52-4157-98dc-ceef89b916f3?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f0d86481-3f52-4157-98dc-ceef89b916f3) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:ce1bf294-3fc8-43c7-80f1-58b5f7736d18 --> [](ce1bf294-3fc8-43c7-80f1-58b5f7736d18) ########## superset-frontend/src/components/ErrorMessage/DatabaseErrorMessage.tsx: ########## @@ -40,20 +43,32 @@ export function DatabaseErrorMessage({ const isVisualization = ['dashboard', 'explore'].includes(source || ''); const [firstLine, ...remainingLines] = message.split('\n'); - const alertMessage = firstLine; const alertDescription = remainingLines.length > 0 ? remainingLines.join('\n') : null; + let alertMessage: ReactNode = firstLine; - const body = extra && ( + if (Array.isArray(extra?.custom_doc_links)) { + alertMessage = ( + <> + {firstLine} + {extra.custom_doc_links.map(link => ( + <div key={link.url}> + <CustomDocLink {...link} /> + </div> + ))} + </> + ); + } + + const body = extra && extra.show_issue_info !== false && ( <> <p> {t('This may be triggered by:')} <br /> - {extra.issue_codes - ?.map<ReactNode>(issueCode => ( - <IssueCode {...issueCode} key={issueCode.code} /> - )) - .reduce((prev, curr) => [prev, <br />, curr])} + {extra.issue_codes?.flatMap((issueCode, idx, arr) => [ Review Comment: ### Unsafe Issue Codes Access <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The code assumes issue_codes array exists when show_issue_info is true, but it's not guaranteed by the type system or runtime checks. ###### Why this matters This could lead to runtime errors if issue_codes is undefined while show_issue_info is true. ###### Suggested change ∙ *Feature Preview* Add a check for issue_codes existence: ```typescript const body = extra && extra.show_issue_info !== false && extra.issue_codes?.length > 0 && ( <> <p> {t('This may be triggered by:')} <br /> {extra.issue_codes.flatMap((issueCode, idx, arr) => [ <IssueCode {...issueCode} key={issueCode.code} />, idx < arr.length - 1 ? <br key={`br-${issueCode.code}`} /> : null, ])} </p> {/* ... rest of the code ... */} </> ); ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e58ef129-6c93-4479-9ed8-2c5c31283f8a/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e58ef129-6c93-4479-9ed8-2c5c31283f8a?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e58ef129-6c93-4479-9ed8-2c5c31283f8a?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e58ef129-6c93-4479-9ed8-2c5c31283f8a?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e58ef129-6c93-4479-9ed8-2c5c31283f8a) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:4e42a59e-1400-46e2-b972-5ea4f935ab90 --> [](4e42a59e-1400-46e2-b972-5ea4f935ab90) -- 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]
