ci(lint): phase-2 scoped unicorn and sonarjs strict packs#113
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Reviewer's GuideAdds scoped strict ESLint packs using eslint-plugin-unicorn and eslint-plugin-sonarjs for production source files, fixes the minimal code issues surfaced by the new rules, and locks the configuration behavior with unit tests and documentation updates. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Review Summary by Qodo
WalkthroughsDescription• Add eslint-plugin-unicorn and eslint-plugin-sonarjs with curated strict rules
• Scope strict packs to src/**/*.{js,jsx,ts,tsx} excluding test files
• Fix collapsible-if violations in FileAnalyzer and App components
• Add comprehensive test suite for eslint config validation
Diagramflowchart LR
A["ESLint Config"] -->|"Add plugins"| B["sonarjs + unicorn"]
B -->|"Define overrides"| C["Scoped to src/ production code"]
C -->|"Exclude tests"| D["Test files filtered out"]
E["Source violations"] -->|"Fix collapsible-if"| F["FileAnalyzer.ts + App.tsx"]
G["Test coverage"] -->|"Validate config"| H["eslint-config.test.js"]
File Changes1. .eslintrc.js
|
📝 WalkthroughWalkthroughThis PR introduces two new ESLint plugins (sonarjs and unicorn) to the project by adding development dependencies, integrating them into the ESLint configuration with specific rules, and establishing unit tests to validate the configuration. It also includes minor conditional logic refactoring in two source files. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.16.0)eslint.config.js┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m �[1m Loading rules from local config...�[0m src/renderer/components/App.tsx┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m �[1m Loading rules from local config...�[0m tests/unit/scripts/eslint-config.test.js┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m �[1m Loading rules from local config...�[0m Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @Mehdi-Bl, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly upgrades the project's linting capabilities by incorporating Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Dependency ReviewThe following issues were found:
Scanned Files
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully introduces eslint-plugin-unicorn and eslint-plugin-sonarjs to enhance code quality, along with a new test to lock down the configuration. The code changes to comply with the new linting rules are well-executed. I have provided a couple of suggestions for improvement: one to make the new test more robust and another for a minor performance optimization in App.tsx. Overall, this is a valuable addition to the project.
| folders.push(item.path); | ||
| folders = [...folders, ...getAllSubFolders(item)]; |
There was a problem hiding this comment.
This is a good refactoring to collapse the if statements. For a further improvement, you can combine the two push operations and avoid creating a new array in each loop iteration. Using folders.push(item.path, ...getAllSubFolders(item)) is more performant as it avoids the overhead of creating a new array with [...folders, ...].
| folders.push(item.path); | |
| folders = [...folders, ...getAllSubFolders(item)]; | |
| folders.push(item.path, ...getAllSubFolders(item)); |
| expect(strictnessOverride).toBeDefined(); | ||
| expect(strictnessOverride.excludedFiles).toEqual( | ||
| expect.arrayContaining([ | ||
| 'src/**/__tests__/**', | ||
| 'src/**/*.test.{js,jsx,ts,tsx}', | ||
| 'src/**/*.spec.{js,jsx,ts,tsx}', | ||
| ]) | ||
| ); |
There was a problem hiding this comment.
The test 'scopes strict packs to source files and excludes test files' correctly verifies the excludedFiles property, but it doesn't assert that the files glob is what's expected. To make this test more robust and fully align with its description, you should also assert the value of the files property. This will ensure that the strict rules are applied to the correct set of source files and prevent accidental changes to this glob.
expect(strictnessOverride).toBeDefined();
expect(strictnessOverride.files).toEqual(['src/**/*.{js,jsx,ts,tsx}']);
expect(strictnessOverride.excludedFiles).toEqual(
expect.arrayContaining([
'src/**/__tests__/**',
'src/**/*.test.{js,jsx,ts,tsx}',
'src/**/*.spec.{js,jsx,ts,tsx}',
])
);
Code Review by Qodo
✅ 1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| { | ||
| files: ['src/**/*.{js,jsx,ts,tsx}'], | ||
| excludedFiles: [ | ||
| 'src/**/__tests__/**', | ||
| 'src/**/*.test.{js,jsx,ts,tsx}', | ||
| 'src/**/*.spec.{js,jsx,ts,tsx}', | ||
| ], | ||
| rules: { | ||
| 'sonarjs/no-collapsible-if': 'error', | ||
| 'sonarjs/no-identical-conditions': 'error', | ||
| 'sonarjs/no-identical-expressions': 'error', | ||
| 'sonarjs/no-ignored-return': 'error', | ||
| 'sonarjs/no-inverted-boolean-check': 'error', | ||
| 'unicorn/no-array-callback-reference': 'error', | ||
| 'unicorn/no-invalid-fetch-options': 'error', | ||
| 'unicorn/prefer-array-some': 'error', | ||
| 'unicorn/prefer-optional-catch-binding': 'error', | ||
| 'unicorn/prefer-string-starts-ends-with': 'error', | ||
| }, | ||
| }, |
There was a problem hiding this comment.
1. Strict packs not enforced 🐞 Bug ✓ Correctness
The PR adds sonarjs/unicorn strict-pack rules to .eslintrc.js, but the project uses ESLint v9 with a flat config (eslint.config.js) and CI runs eslint src tests without pointing at .eslintrc.js. This likely means the new strict rules are not actually applied in the lint gate, undermining the PR’s intent.
Agent Prompt
### Issue description
Strict-pack lint rules were added to `.eslintrc.js`, but the repo appears to use ESLint v9 flat config (`eslint.config.js`) for `npm run lint`. This likely means the new sonarjs/unicorn rules are not enforced in CI.
### Issue Context
- `npm run lint` runs `eslint src tests ...` without selecting `.eslintrc.js`.
- ESLint v9 + committed `eslint.config.js` implies flat config is active.
- `eslint.config.js` currently does not load `eslint-plugin-sonarjs` or `eslint-plugin-unicorn`, and does not apply the strict-pack rule set.
### Fix Focus Areas
- eslint.config.js[1-70]
- eslint.config.js[47-80]
- .eslintrc.js[1-82]
- tests/unit/scripts/eslint-config.test.js[1-53]
### Suggested approach (flat config)
1. In `eslint.config.js`, `require('eslint-plugin-sonarjs')` and `require('eslint-plugin-unicorn')`.
2. Add a new config block scoped to `src/**/*.{js,jsx,ts,tsx}` with `ignores` for:
- `src/**/__tests__/**`
- `src/**/*.test.{js,jsx,ts,tsx}`
- `src/**/*.spec.{js,jsx,ts,tsx}`
3. In that block, set `plugins: { sonarjs: sonarjsPlugin, unicorn: unicornPlugin }` and add the selected rules.
4. Decide whether to keep `.eslintrc.js` (editor-only) or remove/stop evolving it to avoid divergence.
5. Update the unit test to validate the effective config (`eslint.config.js`) rather than `.eslintrc.js`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
|
Addressed review feedback in commit d872f1d.
Local validation rerun:
|
1 participant
Summary
eslint-plugin-unicornandeslint-plugin-sonarjssrc/**/*.{js,jsx,ts,tsx}while excluding test files to keep noise lowsonarjs/no-collapsible-if)tests/unit/scripts/eslint-config.test.jsto lock strict-pack config behavior and scopetests/catalog.mdwith the new test targetWhy this scope
Validation
npm run lintnpm test -- --runInBandnpm run sonar(attempted locally, blocked by401 Unauthorizeddue unavailable valid Sonar token in shell vault context; CI Sonar gate will validate in PR)Summary by Sourcery
Introduce scoped strict ESLint packs for production source files using sonarjs and unicorn, and update code and tests to comply with the new configuration.
Enhancements:
Build:
Tests:
Summary by CodeRabbit
Refactor
Tests
Chores