korbit-ai[bot] commented on code in PR #31761:
URL: https://github.com/apache/superset/pull/31761#discussion_r1908136917
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/utils/sandbox.ts:
##########
@@ -19,6 +19,7 @@
// A safe alternative to JS's eval
import vm, { Context, RunningScriptOptions } from 'vm';
import _ from 'underscore';
+/* eslint-ignore-next-line no-restricted-syntax*/
import * as d3array from 'd3-array';
Review Comment:
### Invalid ESLint Disable Comment <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The ESLint disable comment has a typo ('eslint-ignore-next-line' instead of
'eslint-disable-next-line'), making it ineffective at disabling the intended
ESLint rule.
###### Why this matters
The ESLint rule intended to be disabled will still be active, potentially
causing build errors or CI failures when the code is pushed.
###### Suggested change
Correct the ESLint disable comment syntax:
```typescript
/* eslint-disable-next-line no-restricted-syntax */
import * as d3array from 'd3-array';
```
</details>
###### Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help
Korbit improve your reviews.
<!--- korbi internal id:4b186e7e-83cb-4e77-ba7b-e1d6e1acb6a3 -->
##########
superset-frontend/src/SqlLab/components/ScheduleQueryButton/index.tsx:
##########
@@ -50,7 +50,7 @@ const getJSONSchema = () => {
jsonSchema.properties[key] = {
...value,
default: value.default
- ? chrono.parseDate(value.default)?.toISOString()
+ ? parseDate(value.default)?.toISOString()
Review Comment:
### Unsafe Date Parsing <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Using optional chaining with parseDate result could lead to runtime errors
if parseDate returns undefined and toISOString() is called.
###### Why this matters
If chrono-node's parseDate fails to parse the date string, it returns
undefined. The optional chaining will prevent accessing toISOString on
undefined, but the code still assigns undefined to the default property which
might cause issues when the form tries to use this value.
###### Suggested change
Add additional null check and provide a fallback value:
```typescript
default: value.default && parseDate(value.default)
? parseDate(value.default).toISOString()
: null,
```
</details>
###### Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help
Korbit improve your reviews.
<!--- korbi internal id:239da8a5-10ea-4c8f-a0c6-2030bcf33c94 -->
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/utils/sandbox.ts:
##########
@@ -19,6 +19,7 @@
// A safe alternative to JS's eval
import vm, { Context, RunningScriptOptions } from 'vm';
import _ from 'underscore';
+/* eslint-ignore-next-line no-restricted-syntax*/
import * as d3array from 'd3-array';
Review Comment:
### Avoid ignoring ESLint rule 'no-restricted-syntax'. <sub></sub>
<details>
<summary>Tell me more</summary>
​
Hello, thank you for your pull request. I notice that you ignored an ESLint
rule with 'eslint-ignore-next-line no-restricted-syntax'. As much as possible,
we should follow the lint rules because they help to keep our codebase
consistent, readable, and maintainable. The rule you've ignored might be in
place to deter us from importing the whole 'd3-array' module – which could
potentially increase the bundle size. Could you please refactor your code to
comply with the rule by importing only the specific functions you need from
'd3-array'? Thank you.
</details>
###### Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help
Korbit improve your reviews.
<!--- korbi internal id:34f50f2a-6a84-4a9c-99cb-3c5e2e0e33db -->
--
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]
