codeant-ai-for-open-source[bot] commented on PR #36433: URL: https://github.com/apache/superset/pull/36433#issuecomment-3614353111
## Nitpicks 🔍 <table> <tr><td>🔒 <strong>No security issues identified</strong></td></tr> <tr><td>⚡ <strong>Recommended areas for review</strong><br><br> - [ ] <a href='https://github.com/apache/superset/pull/36433/files#diff-a5ea9e53124e339e53389f49e79c649249e961b66e8f3ab2aa4a81e937e1182dR172-R175'><strong>Questionable Logic</strong></a><br>When dependents are found in package-lock, analyze_dependency sets status 'transitive-only' and recommendation 'remove'. This is counterintuitive: if other packages depend on this package, removing it may break those dependents or conversely it may be safe to remove if it's provided transitively. This behavior should be documented or re-checked to ensure the recommendation is correct.<br> - [ ] <a href='https://github.com/apache/superset/pull/36433/files#diff-a5ea9e53124e339e53389f49e79c649249e961b66e8f3ab2aa4a81e937e1182dR25-R28'><strong>Missing file handling</strong></a><br>Opening package.json and package-lock.json is done without checking existence or handling JSON decoding errors. If the provided --path is incorrect or files are missing/corrupt the script will raise an unhandled exception. Consider early checks and clearer error messages or safe fallbacks.<br> - [ ] <a href='https://github.com/apache/superset/pull/36433/files#diff-a5ea9e53124e339e53389f49e79c649249e961b66e8f3ab2aa4a81e937e1182dR129-R134'><strong>Fragile config file reading</strong></a><br>The code opens all matching config files without try/except; unreadable files or directories (or binary/large files) could raise exceptions. Also glob patterns may match unexpected files. Consider guarding file reads and skipping non-regular files.<br> - [ ] <a href='https://github.com/apache/superset/pull/36433/files#diff-8fbacc899442df6105f4e46858b17060e2f0b8d75b734efe924fd4f23ba376a1R19-R27'><strong>Build / config residue</strong></a><br>Ensure all build-tooling and config (SWC/Babel/webpack) have been updated consistently to use React Refresh instead of react-hot-loader. Confirm there are no remaining references or plugins that assume react-hot-loader (imports, babel plugins, vendor bundles, or webpack aliases).<br> - [ ] <a href='https://github.com/apache/superset/pull/36433/files#diff-40640739c760fc10b2c86514e363f5d2932d1c73b2388d5ecf58fbe10ca23aabR19-R26'><strong>Removed import audit</strong></a><br>The import for `react-hot-loader` was removed. Make sure there are no remaining references elsewhere in the codebase or build config (Babel, webpack, vendor lists). A quick search and CI build check should confirm there are no missing symbols.<br> - [ ] <a href='https://github.com/apache/superset/pull/36433/files#diff-8fbacc899442df6105f4e46858b17060e2f0b8d75b734efe924fd4f23ba376a1R99-R112'><strong>HMR behavior change</strong></a><br>Removing the `react-hot-loader` wrapper and exporting the raw `App` relies on React Refresh behavior. Verify that HMR preserves component state and update behavior across the app (particularly for any class components and legacy code paths that relied on react-hot-loader's advanced state-preservation behavior).<br> - [ ] <a href='https://github.com/apache/superset/pull/36433/files#diff-a5ea9e53124e339e53389f49e79c649249e961b66e8f3ab2aa4a81e937e1182dR41-R82'><strong>Performance Issue</strong></a><br>The search_for_imports routine invokes ripgrep/grep once per pattern per source path per package. For large dependency lists this results in many separate subprocess calls and repeated scanning of the same directories, which is slow. Consider batching patterns into a single rg invocation or searching once per package and post-filtering results.<br> - [ ] <a href='https://github.com/apache/superset/pull/36433/files#diff-40640739c760fc10b2c86514e363f5d2932d1c73b2388d5ecf58fbe10ca23aabR46-R51'><strong>Empty dev block</strong></a><br>The development-only conditional no longer executes any code and only contains a comment. This is a code smell (and can trigger linter warnings). Either remove the conditional entirely or make it do a minimal runtime check (e.g. verify HMR is available) so the intent is explicit.<br> </td></tr> </table> -- 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]
