Eslint upgrade to v9 #9344
Eslint upgrade to v9 #9344
Conversation
WalkthroughThe changes in this pull request involve the removal of the Changes
Assessment against linked issues
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
eslint.config.mjs (1)
15-26: Consider adding common directories to the ignore listThe current
ignoreslist excludes several directories and file types. To further optimize linting performance, consider addingnode_modules,.cache, andcoveragedirectories to the ignore list.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
.eslintrc.json(0 hunks)eslint.config.mjs(1 hunks)package.json(4 hunks)
💤 Files with no reviewable changes (1)
- .eslintrc.json
🔇 Additional comments (4)
eslint.config.mjs (2)
66-67: Verify the parserOptions.project path
Ensure that the project path in parserOptions ("./tsconfig.json") correctly references your TypeScript configuration file. An incorrect path can lead to linting errors or reduced performance.
139-139:
Verify the spread operator usage in MDX configuration
Using the spread operator with mdxPlugin.configs.recommended may not work as expected. Confirm that this syntax is supported in the ESLint flat config. If issues arise, consider importing and extending the recommended configuration.
package.json (2)
63-63: Addition of @radix-ui/react-icons is appropriate
The new dependency @radix-ui/react-icons has been added. Ensure it's used in the project to justify its inclusion.
108-110: ESLint and related packages updated successfully
The updates to ESLint and its plugins align with the upgrade to ESLint v9. The inclusion of new packages like @eslint/compat, @eslint/eslintrc, and globals supports the new configuration.
Also applies to: 128-129, 136-136, 138-138, 142-142, 144-144
package.json
Outdated
| @@ -81,6 +81,7 @@ | |||
| "dayjs": "^1.11.13", | |||
| "echarts": "^5.5.1", | |||
| "echarts-for-react": "^3.0.2", | |||
| "eslint-plugin-markdown": "^5.1.0", | |||
There was a problem hiding this comment.
Move eslint-plugin-markdown to devDependencies
eslint-plugin-markdown is a development tool and should be listed under devDependencies to prevent it from being included in the production build.
Apply this diff to fix the issue:
"dependencies": {
- "eslint-plugin-markdown": "^5.1.0",
},
"devDependencies": {
+ "eslint-plugin-markdown": "^5.1.0",Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
@amjithtitus09 this package is not required
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
eslint.config.mjs (2)
26-50: Consider adding TypeScript-specific compiler optionsWhile the global settings are well-configured, consider adding TypeScript compiler options for better type checking:
languageOptions: { ecmaVersion: 12, sourceType: "module", + parserOptions: { + project: "./tsconfig.json", + tsconfigRootDir: ".", + }, globals: { ...globals.browser, ...globals.node, }, parserOptions: { ecmaFeatures: { jsx: true, }, }, },
116-117: Simplify Prettier configurationThe Prettier plugin automatically sets up both the plugin and config when using
eslint-plugin-prettier/recommended.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
eslint.config.mjs(1 hunks)package.json(4 hunks)src/components/Facility/AssetCreate.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🪛 eslint
eslint.config.mjs
[error] 10-11: Delete ⏎
(prettier/prettier)
[error] 118-118: Insert ⏎
(prettier/prettier)
🔇 Additional comments (5)
eslint.config.mjs (2)
1-24: LGTM! Base configuration follows ESLint v9 best practices
The imports and base configuration are well-structured, following the new flat config format introduced in ESLint v9.
🧰 Tools
🪛 eslint
[error] 10-11: Delete ⏎
(prettier/prettier)
65-66: Avoid spreading recommended rules directly
Spreading tseslint.configs.recommended.rules and plugin rules can cause conflicts or unexpected behavior.
Also applies to: 86-87
src/components/Facility/AssetCreate.tsx (3)
163-169: LGTM! Improved readability of section visibility logic
The refactored implementation using direct conditional statements is clearer and maintains the same functionality.
196-207: LGTM! Enhanced null safety in asset data handling
The addition of null checks before setting state values improves type safety and prevents potential runtime errors.
499-499: LGTM! Improved className handling
The use of template literals for dynamic class assignments improves code readability and maintainability.
|
Hey @sainak, I've made the requested changes |
package.json
Outdated
| @@ -81,6 +81,7 @@ | |||
| "dayjs": "^1.11.13", | |||
| "echarts": "^5.5.1", | |||
| "echarts-for-react": "^3.0.2", | |||
| "eslint-plugin-markdown": "^5.1.0", | |||
There was a problem hiding this comment.
@amjithtitus09 this package is not required
|
@amjithtitus09 lint is failing in your PR, do check it |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
eslint.config.mjs (2)
54-78: 🛠️ Refactor suggestionRefactor TypeScript configuration to use extends
The TypeScript configuration should use
extendsinstead of spreading rules to avoid potential conflicts.Apply this diff to improve the configuration:
{ files: ["**/*.{ts,tsx}"], plugins: { "@typescript-eslint": tseslint, }, languageOptions: { parser: tsParser, parserOptions: { project: "./tsconfig.json", }, }, + extends: [ + "@typescript-eslint/recommended" + ], rules: { - ...tseslint.configs.recommended.rules, "@typescript-eslint/no-unused-vars": [ "error", { argsIgnorePattern: "^_", varsIgnorePattern: "^_", caughtErrorsIgnorePattern: "^_", }, ], "@typescript-eslint/no-explicit-any": "warn", "no-undef": "off", }, }
80-92: 🛠️ Refactor suggestionRefactor React configuration to use extends
The React configuration should use
extendsinstead of spreading rules to avoid maintenance challenges.Apply this diff to improve the configuration:
{ files: ["**/*.{jsx,tsx}"], plugins: { react: reactPlugin, "react-hooks": reactHooksPlugin, }, + extends: [ + "plugin:react/recommended", + "plugin:react-hooks/recommended" + ], rules: { - ...reactPlugin.configs.recommended.rules, - ...reactHooksPlugin.configs.recommended.rules, "react/prop-types": "off", }, }
🧹 Nitpick comments (2)
src/Locale/update_locale.js (1)
63-66: Enhance error handling with more contextThe current error handling discards the original error information which could be useful for debugging.
Apply this diff to improve error handling:
try { files[file] = JSON.parse(readFile(`./${folderName}/${file}`)); - } catch { - throw new Error(`Cannot parse ${file} file!`); + } catch (error) { + throw new Error(`Cannot parse ${file} file: ${error.message}`); }src/components/Common/ExcelFIleDragAndDrop.tsx (1)
Line range hint
119-127: Consider enhancing file validationWhile the current file type validation works, consider these improvements:
- Add file size limits to prevent large uploads
- Use MIME type checking in addition to file extension validation
- Internationalize error messages for consistency with other translations
Example implementation:
const fileTypes = [ "vnd.ms-excel", "text/csv", "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet", ]; +const MAX_FILE_SIZE = 5 * 1024 * 1024; // 5MB if (!fileTypes.includes(droppedFile?.type)) { - dragProps.setFileDropError("Please drop a Excel / CSV file to upload!"); + dragProps.setFileDropError(t("errors.invalid_file_type")); setSelectedFile(null); return; } + +if (droppedFile.size > MAX_FILE_SIZE) { + dragProps.setFileDropError(t("errors.file_too_large")); + setSelectedFile(null); + return; +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
eslint.config.mjs(1 hunks)src/CAREUI/interactive/SlideOver.tsx(1 hunks)src/Locale/update_locale.js(1 hunks)src/PluginEngine.tsx(0 hunks)src/Providers/HistoryAPIProvider.tsx(1 hunks)src/components/Common/ExcelFIleDragAndDrop.tsx(4 hunks)src/components/Common/Sidebar/Sidebar.tsx(1 hunks)src/components/Resource/ResourceBoard.tsx(1 hunks)src/service-worker.ts(0 hunks)
💤 Files with no reviewable changes (2)
- src/PluginEngine.tsx
- src/service-worker.ts
🧰 Additional context used
🪛 eslint
eslint.config.mjs
[error] 10-11: Delete ⏎
(prettier/prettier)
[error] 120-120: Insert ⏎
(prettier/prettier)
🔇 Additional comments (9)
eslint.config.mjs (2)
1-24: LGTM: Base configuration follows ESLint v9 flat config format
The base configuration and imports are well-structured. The ignore patterns are comprehensive and cover common build/output directories.
🧰 Tools
🪛 eslint
[error] 10-11: Delete ⏎
(prettier/prettier)
94-116: 🛠️ Refactor suggestion
Refactor i18next configuration to use extends
Follow the same pattern as other plugins by using extends instead of spreading rules.
Apply this diff to improve the configuration:
{
files: ["**/*.{js,jsx,ts,tsx}"],
plugins: {
i18next: i18nextPlugin,
},
+ extends: [
+ "plugin:i18next/recommended"
+ ],
rules: {
- ...i18nextPlugin.configs.recommended.rules,
"i18next/no-literal-string": [
"warn",
{
mode: "jsx-only",
"jsx-attributes": {
include: ["label", "placeholder", "error", "title"],
exclude: [".*"],
},
callees: {
exclude: [".*"],
},
},
],
},
}Likely invalid or redundant comment.
src/Providers/HistoryAPIProvider.tsx (1)
Line range hint 1-40: LGTM: Component follows React best practices
The implementation is clean and follows React patterns. The new ResetHistoryContext is properly integrated.
src/CAREUI/interactive/SlideOver.tsx (1)
119-119: LGTM: Clean optional chaining implementation
The change from explicit null check to optional chaining operator improves code conciseness while maintaining the same functionality and type safety.
src/components/Resource/ResourceBoard.tsx (1)
44-44: Whitespace change
This is a minor formatting change with no functional impact.
src/components/Common/Sidebar/Sidebar.tsx (1)
209-209: LGTM: Clean context definition
The removal of unnecessary whitespace and comments improves code cleanliness while maintaining the same functionality.
src/components/Common/ExcelFIleDragAndDrop.tsx (3)
50-50: LGTM: Proper handling of optional callbacks
The addition of optional chaining operators for onClose and setIsValid callbacks aligns with their optional status in the Props interface and ensures type-safe function calls.
Also applies to: 101-101
202-204: LGTM: Consistent handling of optional callback
The optional chaining for setIsValid is correctly applied in the cancel handler, maintaining consistency with other usages and ensuring type safety.
232-232: LGTM: Enhanced security for external link
The addition of rel="noreferrer" is a security best practice that prevents the opened page from accessing the window.opener property, protecting against potential reverse tabnabbing attacks.
|
👋 Hi, @amjithtitus09, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
|
@amjithtitus09 you can try to fix the easily fixable errors, and change the rest to warnings instead of errors temporarily along with creating issues to fix the errors and revert the rule change |
|
👋 Hi, @amjithtitus09, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
5ceeaba to
0291d9c
Compare
|
@amjithtitus09 why was this pr closed? |
|
@sainak I discarded the commits by mistake and the PR got closed. Opening another one with existing commits |
|
Changes moved to #9538 |
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Chores
AssetCreate,ExcelFileDragAndDrop, andSidebar.