korbit-ai[bot] commented on code in PR #31907:
URL: https://github.com/apache/superset/pull/31907#discussion_r1920391638
##########
superset-frontend/src/SqlLab/components/SaveDatasetModal/index.tsx:
##########
@@ -280,7 +280,7 @@ export const SaveDatasetModal = ({
// Remove the special filters entry from the templateParams
// before saving the dataset.
let templateParams;
- if (isString(datasource?.templateParams)) {
+ if (typeof datasource?.templateParams === 'string') {
const p = JSON.parse(datasource.templateParams);
Review Comment:
### Unhandled JSON Parse Error <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The code doesn't handle JSON.parse errors which could occur if
templateParams contains invalid JSON string.
###### Why this matters
If templateParams contains malformed JSON, the application will crash when
trying to parse it, interrupting the save dataset operation.
###### Suggested change ∙ *Feature Preview*
Add try-catch block to handle potential JSON parsing errors:
```typescript
let templateParams;
if (typeof datasource?.templateParams === 'string') {
try {
const p = JSON.parse(datasource.templateParams);
/* eslint-disable-next-line no-underscore-dangle */
if (p._filters) {
/* eslint-disable-next-line no-underscore-dangle */
delete p._filters;
// eslint-disable-next-line no-param-reassign
templateParams = JSON.stringify(p);
}
} catch (error) {
console.error('Error parsing templateParams:', error);
templateParams = datasource.templateParams;
}
}
```
</details>
###### Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help
Korbit improve your reviews.
<!--- korbi internal id:5a90b683-bc70-409a-800e-f02e735d51a8 -->
##########
superset-frontend/src/explore/components/controls/DateFilterControl/components/CustomFrame.tsx:
##########
@@ -76,7 +75,7 @@ export function CustomFrame(props: FrameComponentProps) {
value: string | number,
) {
// only positive values in grainValue controls
- if (isInteger(value) && value > 0) {
+ if (typeof value === 'number' && Number.isInteger(value) && value > 0) {
Review Comment:
### Redundant Type Checking <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Multiple type checks are being performed unnecessarily in sequence, with
Number.isInteger() already implying the value is a number.
###### Why this matters
The redundant type checking adds unnecessary computation overhead,
especially in frequently executed code paths.
###### Suggested change ∙ *Feature Preview*
Simplify the condition to only use Number.isInteger() which inherently
checks for number type:
```typescript
if (Number.isInteger(value) && value > 0)
```
</details>
###### Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help
Korbit improve your reviews.
<!--- korbi internal id:06c0e3e9-65b6-47c0-b5e8-4a0e43ffba4a -->
--
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]
