bito-code-review[bot] commented on code in PR #43686:
URL: https://github.com/apache/superset/pull/43686#discussion_r3887155538
##########
superset-frontend/scripts/bundle-size-summary.js:
##########
@@ -79,8 +80,11 @@ function main() {
console.log(JSON.stringify(results, null, 2));
}
-if (require.main === module) {
+const isMain =
+ process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href;
+
+if (isMain) {
main();
}
-module.exports = { entrypointSizeByExt, main, TRACKED_ENTRYPOINTS };
+export default { entrypointSizeByExt, main, TRACKED_ENTRYPOINTS };
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>ESM export breaks test</b></div>
<div id="fix">
Converting to `export default` breaks the existing test at
`spec/scripts/bundle-size-summary.test.js:20-23`, which still destructures
named exports from `require('../../scripts/bundle-size-summary')`. Under
babel-jest's `@babel/plugin-transform-modules-commonjs`, `export default`
becomes `exports.default`, so `entrypointSizeByExt`/`main` are `undefined` and
every test throws. Update the test to read `.default`.
</div>
</div>
<small><i>Code Review Run #69ff65</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/scripts/check-storybook-coverage.js:
##########
@@ -117,7 +120,7 @@ function main() {
for (const pluginDir of pluginDirs) {
const fullPath = path.join(ROOT, pluginDir);
- if (!fs.existsSync(fullPath)) continue;
+ if (!fs.existsglob.sync(fullPath)) continue;
const pluginName = getPluginName(pluginDir);
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Invalid fs method crash</b></div>
<div id="fix">
`fs.existsglob` is not a method of Node's `fs` module (only
`existsSync`/`statSync` exist). Calling `fs.existsglob.sync(...)` throws
`TypeError: Cannot read properties of undefined`, crashing the plugin loop on
the first directory. Use `fs.existsSync(fullPath)`.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
for (const pluginDir of pluginDirs) {
const fullPath = path.join(ROOT, pluginDir);
if (!fs.existsSync(fullPath)) continue;
const pluginName = getPluginName(pluginDir);
````
</div>
</details>
</div>
<small><i>Code Review Run #69ff65</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/scripts/oxlint-metrics-uploader.js:
##########
@@ -280,8 +280,8 @@ async function runOxlintAndProcess() {
// Run the process, unless this file was imported (e.g. by a test) rather than
// executed, in which case nothing should be linted or uploaded on import.
-if (require.main === module) {
+if (__filename === process.argv[1]) {
runOxlintAndProcess().catch(console.error);
}
-module.exports = { parseRuleId };
+export default { parseRuleId };
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Test require ESM break</b></div>
<div id="fix">
In the test file, replace the
`require('../../scripts/oxlint-metrics-uploader')` call with `import uploader
from '../../scripts/oxlint-metrics-uploader';` and update assertions to call
`uploader.parseRuleId`, so the ESM module can be loaded without ERR_REQUIRE_ESM.
</div>
</div>
<small><i>Code Review Run #69ff65</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/scripts/check-storybook-coverage.js:
##########
@@ -150,7 +153,10 @@ function main() {
for (const componentDir of componentDirs) {
const fullPath = path.join(ROOT, componentDir);
- if (!fs.existsSync(fullPath) || !fs.statSync(fullPath).isDirectory())
+ if (
+ !fs.existsglob.sync(fullPath) ||
+ !fs.statglob.sync(fullPath).isDirectory()
+ )
continue;
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Invalid fs method crash</b></div>
<div id="fix">
Same invalid methods here: `fs.existsglob`/`fs.statglob` do not exist on
Node's `fs`. `fs.statglob.sync(...)` throws `TypeError` on the first core
component directory. Use `fs.existsSync(fullPath)` and
`fs.statSync(fullPath).isDirectory()`.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
for (const componentDir of componentDirs) {
const fullPath = path.join(ROOT, componentDir);
if (
!fs.existsSync(fullPath) ||
!fs.statSync(fullPath).isDirectory()
)
continue;
````
</div>
</details>
</div>
<small><i>Code Review Run #69ff65</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/scripts/oxlint-metrics-uploader.js:
##########
@@ -280,8 +280,8 @@ async function runOxlintAndProcess() {
// Run the process, unless this file was imported (e.g. by a test) rather than
// executed, in which case nothing should be linted or uploaded on import.
-if (require.main === module) {
+if (__filename === process.argv[1]) {
runOxlintAndProcess().catch(console.error);
}
-module.exports = { parseRuleId };
+export default { parseRuleId };
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>ESM __filename crash</b></div>
<div id="fix">
This file is now an ES module (`"type": "module"` in package.json), but
`__filename` is a CommonJS global that is undefined in ESM and throws
`ReferenceError` when referenced, crashing `npm run lint-stats` on every run.
Even if defined, comparing absolute `__filename` to relative `process.argv[1]`
would be false, so `runOxlintAndProcess()` would never execute. Use
`import.meta.url === pathToFileURL(process.argv[1]).href`.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
import { pathToFileURL } from 'node:url';
// Run the process, unless this file was imported (e.g. by a test) rather
than
// executed, in which case nothing should be linted or uploaded on import.
if (import.meta.url === pathToFileURL(process.argv[1]).href) {
runOxlintAndProcess().catch(console.error);
}
export default { parseRuleId };
````
</div>
</details>
</div>
<small><i>Code Review Run #69ff65</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
