korbit-ai[bot] commented on code in PR #34760:
URL: https://github.com/apache/superset/pull/34760#discussion_r2286059049
##########
superset-frontend/src/pages/ChartList/index.tsx:
##########
@@ -387,14 +387,21 @@ function ChartList(props: ChartListProps) {
datasource_url: dsUrl,
},
},
- }: any) => (
- <Tooltip title={dsNameTxt} placement="top">
- {/* dsNameTxt can be undefined, schema.name or just name */}
- <GenericLink to={dsUrl}>
- {dsNameTxt ? dsNameTxt.split('.')[1] || dsNameTxt : ''}
- </GenericLink>
- </Tooltip>
- ),
+ }: any) => {
+ // Extract dataset name from datasource_name_text
+ // Format can be "schema.name" or just "name"
+ const displayName = dsNameTxt
+ ? dsNameTxt.includes('.')
+ ? dsNameTxt.split('.').slice(1).join('.') // Handle names with
dots
Review Comment:
### Incorrect Dataset Name Parsing <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The code assumes that only the first dot in the dataset name separates the
schema from the name, which could be incorrect if the actual dataset name
contains dots.
###### Why this matters
If a dataset name contains dots (e.g., 'schema.my.table.name'), the current
logic will incorrectly include part of the actual table name in the schema
prefix, leading to improper display of dataset names in the UI.
###### Suggested change ∙ *Feature Preview*
A more robust solution would be to use a schema-aware approach or define a
clear convention for schema separation. One possible solution:
```typescript
const displayName = dsNameTxt
? dsNameTxt.includes('.')
? dsNameTxt.substring(dsNameTxt.indexOf('.') + 1) // Take everything
after first dot
: dsNameTxt // No schema, use the full name
: '';
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/32426627-e140-420f-8394-e5ff4aa8e11e/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/32426627-e140-420f-8394-e5ff4aa8e11e?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/32426627-e140-420f-8394-e5ff4aa8e11e?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/32426627-e140-420f-8394-e5ff4aa8e11e?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/32426627-e140-420f-8394-e5ff4aa8e11e)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:4e490517-13a4-4af5-91b2-e24861931318 -->
[](4e490517-13a4-4af5-91b2-e24861931318)
##########
superset-frontend/src/pages/ChartList/index.tsx:
##########
@@ -387,14 +387,21 @@
datasource_url: dsUrl,
},
},
- }: any) => (
- <Tooltip title={dsNameTxt} placement="top">
- {/* dsNameTxt can be undefined, schema.name or just name */}
- <GenericLink to={dsUrl}>
- {dsNameTxt ? dsNameTxt.split('.')[1] || dsNameTxt : ''}
- </GenericLink>
- </Tooltip>
- ),
+ }: any) => {
+ // Extract dataset name from datasource_name_text
+ // Format can be "schema.name" or just "name"
+ const displayName = dsNameTxt
+ ? dsNameTxt.includes('.')
+ ? dsNameTxt.split('.').slice(1).join('.') // Handle names with
dots
+ : dsNameTxt // No schema, use the full name
+ : '';
Review Comment:
### Dataset Name Parsing Logic Not Abstracted <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The dataset name parsing logic is embedded directly in the render function,
making it harder to reuse and test. This logic handles complex string
manipulation that should be extracted into a separate utility function.
###### Why this matters
Embedding business logic in render functions makes the code less
maintainable, harder to test, and violates the Single Responsibility Principle.
If this parsing logic needs to be used elsewhere, it would need to be
duplicated.
###### Suggested change ∙ *Feature Preview*
```typescript
const parseDatasetDisplayName = (dsNameTxt: string | undefined): string => {
if (!dsNameTxt) return '';
return dsNameTxt.includes('.')
? dsNameTxt.split('.').slice(1).join('.')
: dsNameTxt;
};
// In render function:
const displayName = parseDatasetDisplayName(dsNameTxt);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8357a190-bf55-4880-99a3-bd618c410c91/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8357a190-bf55-4880-99a3-bd618c410c91?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8357a190-bf55-4880-99a3-bd618c410c91?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8357a190-bf55-4880-99a3-bd618c410c91?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8357a190-bf55-4880-99a3-bd618c410c91)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:ae06d99f-8e45-4536-965d-c0b72fbae940 -->
[](ae06d99f-8e45-4536-965d-c0b72fbae940)
--
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]
