korbit-ai[bot] commented on code in PR #33341:
URL: https://github.com/apache/superset/pull/33341#discussion_r2070956796
##########
superset-frontend/src/explore/components/controls/ViewQuery.tsx:
##########
@@ -51,25 +53,92 @@ const StyledSyntaxContainer = styled.div`
flex-direction: column;
`;
+const StyledHeaderMenuContainer = styled.div`
+ display: flex;
+ flex-direction: row;
+ column-gap: ${({ theme }) => theme.gridUnit * 2}px;
+ margin-top: ${({ theme }) => -theme.gridUnit * 4}px;
+ align-items: baseline;
+`;
+
const StyledSyntaxHighlighter = styled(SyntaxHighlighter)`
flex: 1;
`;
const ViewQuery: FC<ViewQueryProps> = props => {
- const { sql, language = 'sql' } = props;
+ const { sql, language = 'sql', datasource } = props;
+ const datasetId = datasource.split('__')[0];
Review Comment:
### Unexplained Magic String Separator <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Magic string separator '__' is hardcoded without explanation of its
significance in parsing the datasource identifier.
###### Why this matters
Future developers may not understand the format of datasource identifiers,
making maintenance risky and debugging harder.
###### Suggested change ∙ *Feature Preview*
Extract the separator into a named constant with a descriptive name:
```typescript
const DATASOURCE_ID_SEPARATOR = '__';
const datasetId = datasource.split(DATASOURCE_ID_SEPARATOR)[0];
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2fe9cd54-19ac-40da-bcd0-aa3443b507c5/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2fe9cd54-19ac-40da-bcd0-aa3443b507c5?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2fe9cd54-19ac-40da-bcd0-aa3443b507c5?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2fe9cd54-19ac-40da-bcd0-aa3443b507c5?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2fe9cd54-19ac-40da-bcd0-aa3443b507c5)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:949df9d7-c3e9-4e3c-a779-6d982bc2a357 -->
[](949df9d7-c3e9-4e3c-a779-6d982bc2a357)
##########
superset-frontend/src/explore/components/controls/ViewQuery.tsx:
##########
@@ -51,25 +53,92 @@ const StyledSyntaxContainer = styled.div`
flex-direction: column;
`;
+const StyledHeaderMenuContainer = styled.div`
+ display: flex;
+ flex-direction: row;
+ column-gap: ${({ theme }) => theme.gridUnit * 2}px;
+ margin-top: ${({ theme }) => -theme.gridUnit * 4}px;
+ align-items: baseline;
+`;
+
const StyledSyntaxHighlighter = styled(SyntaxHighlighter)`
flex: 1;
`;
const ViewQuery: FC<ViewQueryProps> = props => {
- const { sql, language = 'sql' } = props;
+ const { sql, language = 'sql', datasource } = props;
+ const datasetId = datasource.split('__')[0];
+ const [formattedSQL, setFormattedSQL] = useState<string>();
+ const [showFormatSQL, setShowFormatSQL] = useState(false);
+ const history = useHistory();
+ const currentSQL = (showFormatSQL ? formattedSQL : sql) ?? sql;
+
+ const formatCurrentQuery = useCallback(() => {
+ if (formattedSQL) {
+ setShowFormatSQL(val => !val);
+ } else {
+ const queryParams = rison.encode({
+ keys: ['none'],
+ columns: ['database.backend'],
+ });
+ SupersetClient.get({
+ endpoint: `/api/v1/dataset/${datasetId}?q=${queryParams}`,
+ }).then(({ json }) => {
+ SupersetClient.post({
+ endpoint: `/api/v1/sqllab/format_sql/`,
+ body: JSON.stringify({
+ sql,
+ engine: json.result.database.backend,
+ }),
+ headers: { 'Content-Type': 'application/json' },
+ }).then(({ json }) => {
+ setFormattedSQL(json.result);
+ setShowFormatSQL(true);
+ });
+ });
Review Comment:
### Missing Error Handling in API Calls <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The API calls lack error handling, which could lead to unhandled exceptions
if the network requests fail or return unexpected responses.
###### Why this matters
Users won't receive any feedback if the SQL formatting fails, and the
application might appear unresponsive.
###### Suggested change ∙ *Feature Preview*
Add error handling to the API calls:
```typescript
formatCurrentQuery = useCallback(() => {
if (formattedSQL) {
setShowFormatSQL(val => !val);
} else {
const queryParams = rison.encode({
keys: ['none'],
columns: ['database.backend'],
});
SupersetClient.get({
endpoint: `/api/v1/dataset/${datasetId}?q=${queryParams}`,
})
.then(({ json }) => {
return SupersetClient.post({
endpoint: '/api/v1/sqllab/format_sql/',
body: JSON.stringify({
sql,
engine: json.result.database.backend,
}),
headers: { 'Content-Type': 'application/json' },
});
})
.then(({ json }) => {
setFormattedSQL(json.result);
setShowFormatSQL(true);
})
.catch(error => {
// Handle the error appropriately, e.g., show a notification
console.error('Failed to format SQL:', error);
});
}
}, [sql, datasetId, formattedSQL]);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/46e59f53-7820-400d-91ea-2754a37000a2/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/46e59f53-7820-400d-91ea-2754a37000a2?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/46e59f53-7820-400d-91ea-2754a37000a2?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/46e59f53-7820-400d-91ea-2754a37000a2?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/46e59f53-7820-400d-91ea-2754a37000a2)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:9886e005-bb5b-433d-83f2-debdb8ce548e -->
[](9886e005-bb5b-433d-83f2-debdb8ce548e)
##########
superset-frontend/src/explore/components/controls/ViewQuery.tsx:
##########
@@ -51,25 +53,92 @@ const StyledSyntaxContainer = styled.div`
flex-direction: column;
`;
+const StyledHeaderMenuContainer = styled.div`
+ display: flex;
+ flex-direction: row;
+ column-gap: ${({ theme }) => theme.gridUnit * 2}px;
+ margin-top: ${({ theme }) => -theme.gridUnit * 4}px;
+ align-items: baseline;
+`;
+
const StyledSyntaxHighlighter = styled(SyntaxHighlighter)`
flex: 1;
`;
const ViewQuery: FC<ViewQueryProps> = props => {
- const { sql, language = 'sql' } = props;
+ const { sql, language = 'sql', datasource } = props;
+ const datasetId = datasource.split('__')[0];
+ const [formattedSQL, setFormattedSQL] = useState<string>();
+ const [showFormatSQL, setShowFormatSQL] = useState(false);
+ const history = useHistory();
+ const currentSQL = (showFormatSQL ? formattedSQL : sql) ?? sql;
+
+ const formatCurrentQuery = useCallback(() => {
+ if (formattedSQL) {
+ setShowFormatSQL(val => !val);
+ } else {
+ const queryParams = rison.encode({
+ keys: ['none'],
+ columns: ['database.backend'],
+ });
Review Comment:
### Unexplained API Query Parameters <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Magic values 'none' and 'database.backend' are hardcoded without explanation
of their purpose in the API query.
###### Why this matters
The meaning and necessity of these specific query parameters is unclear,
making the code harder to maintain or modify correctly.
###### Suggested change ∙ *Feature Preview*
Extract these values into named constants with descriptive names:
```typescript
const DATASET_QUERY_PARAMS = {
KEYS: ['none'],
REQUIRED_COLUMNS: ['database.backend'],
};
const queryParams = rison.encode(DATASET_QUERY_PARAMS);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ff8b6658-d817-40eb-a6e8-acfb253f33d2/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ff8b6658-d817-40eb-a6e8-acfb253f33d2?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ff8b6658-d817-40eb-a6e8-acfb253f33d2?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ff8b6658-d817-40eb-a6e8-acfb253f33d2?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ff8b6658-d817-40eb-a6e8-acfb253f33d2)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:a61fa4a9-ef03-45c0-9190-371b8e594489 -->
[](a61fa4a9-ef03-45c0-9190-371b8e594489)
--
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-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
