chore: bump typescript #557
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request includes modifications to various files, primarily focusing on ESLint configuration, Husky pre-commit hooks, and component updates. The Changes
Possibly related PRs
Suggested reviewers
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
- .husky/pre-commit (0 hunks)
- apps/router/.eslintrc.js (0 hunks)
- apps/router/src/api/GuardianApi.ts (1 hunks)
- apps/router/src/gateway-ui/components/federations/ViewConfig.tsx (1 hunks)
- apps/router/src/guardian-ui/components/NumberFormControl.tsx (1 hunks)
- apps/router/src/guardian-ui/components/dashboard/tabs/meta/manager/MetaManager.tsx (1 hunks)
- apps/router/src/guardian-ui/components/setup/screens/roleSelector/RoleSelector.tsx (1 hunks)
- apps/router/src/guardian-ui/utils/api.ts (1 hunks)
- apps/router/src/guardian-ui/utils/index.ts (1 hunks)
- eslint.config.mjs (1 hunks)
- package.json (2 hunks)
- packages/ui/src/NetworkIndicator.tsx (1 hunks)
💤 Files with no reviewable changes (2)
- .husky/pre-commit
- apps/router/.eslintrc.js
🔇 Additional comments (11)
eslint.config.mjs (3)
1-4: Imports and export look good.
5-6: Base configurations are properly included.
7-20: Configuration for all JS/TS files is correct.package.json (1)
17-20: ESLint major version update may introduce breaking changes.The update to ESLint v9 could potentially affect existing lint rules or configurations. Ensure all ESLint-related scripts and configurations are tested thoroughly.
packages/ui/src/NetworkIndicator.tsx (1)
33-33: Error logging added.The change improves error handling in the
isMutinynetfunction.apps/router/src/guardian-ui/components/NumberFormControl.tsx (2)
40-42: Improved readability of onChange handling.The explicit if statement enhances code clarity.
Line range hint
1-85: Verify completeness of changes.The AI summary mentioned modifications to error handling logic, but these are not visible in the provided code.
Run this script to check for additional changes:
apps/router/src/guardian-ui/utils/index.ts (1)
86-86: Approved: Enhanced error logging.The added console error improves debugging capabilities.
apps/router/src/guardian-ui/utils/api.ts (1)
10-15: Type safety improvement approved.The changes enhance type safety without introducing issues.
apps/router/src/guardian-ui/components/setup/screens/roleSelector/RoleSelector.tsx (1)
110-114: Logic change in onClick handlerThe modification improves readability by using an if-else statement instead of a ternary operator. This change maintains the existing functionality while making the code more explicit.
apps/router/src/guardian-ui/components/dashboard/tabs/meta/manager/MetaManager.tsx (1)
133-135: Approved: Improved implementation ofremoveCustomField.The new implementation correctly removes the custom field while adhering to React's immutability principle.
eslint.config.mjs
Outdated
| { | ||
| files: ['**/*.ts', '**/*.tsx'], | ||
| plugins: { | ||
| '@typescript-eslint': tseslint.plugin, | ||
| }, | ||
| languageOptions: { | ||
| parser: tseslint.parser, | ||
| parserOptions: { | ||
| project: './tsconfig.json', | ||
| }, | ||
| }, | ||
| rules: { | ||
| '@typescript-eslint/no-unused-vars': [ | ||
| 'error', | ||
| { argsIgnorePattern: '^_' }, | ||
| ], | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Remove repeated rule in TS-specific configuration.
The unused variables rule is already defined in the previous configuration object which applies to all JS/TS files. It's unnecessary to repeat it here.
Remove lines 32-37 to avoid redundancy.
package.json
Outdated
| "@types/node": "^22.7.4", | ||
| "@vitalets/google-translate-api": "^9.2.0", |
There was a problem hiding this comment.
Incorrect @types/node version and placement.
The @types/node package should be in devDependencies, not dependencies. Also, version ^22.7.4 is incorrect; it should match your Node.js version (likely 18.x or 20.x).
package.json
Outdated
| "ts-node": "^10.9.2", | ||
| "tsup": "^8.3.0", |
There was a problem hiding this comment.
Move ts-node and tsup to devDependencies.
These packages are development tools and should be in the devDependencies section.
package.json
Outdated
| "typescript": "5.5.4", | ||
| "typescript-eslint": "^8.8.0" |
There was a problem hiding this comment.
Incorrect TypeScript version and misplaced typescript-eslint.
TypeScript 5.5.4 doesn't exist yet. Use the latest 5.3.x version. Move typescript-eslint to devDependencies and use the correct version (latest is 6.x).
| @@ -23,8 +23,7 @@ interface ViewConfigModalProps { | |||
| export const ViewConfigModal = forwardRef< | |||
| HTMLButtonElement, | |||
| ViewConfigModalProps | |||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | |||
| >(({ federationId, config }, ref) => { | |||
| >(({ federationId, config }) => { | |||
There was a problem hiding this comment.
Removal of ref forwarding may break existing code.
The ref parameter has been removed from the function signature, indicating that the component no longer supports forwarding refs. This change might break existing code that relies on accessing the ref of this component.
| @@ -96,6 +96,7 @@ export class GuardianApi { | |||
| return true; | |||
| } catch (err) { | |||
| // TODO: make sure error is auth error, not unrelated | |||
| console.error('Failed to authenticate with password:', err); | |||
There was a problem hiding this comment.
Potential security risk in error logging.
Logging the full error object might expose sensitive information. Consider logging only non-sensitive parts of the error.
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (10)
- apps/router/package.json (2 hunks)
- apps/router/src/gateway-ui/components/federations/ViewConfig.tsx (1 hunks)
- eslint.config.mjs (1 hunks)
- package.json (1 hunks)
- packages/eslint-config/package.json (1 hunks)
- packages/types/package.json (1 hunks)
- packages/ui/package.json (1 hunks)
- packages/utils/package.json (1 hunks)
- scripts/add-missing-deps.js (1 hunks)
- turbo.json (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/router/src/gateway-ui/components/federations/ViewConfig.tsx
- eslint.config.mjs
🔇 Additional comments (10)
packages/types/package.json (1)
18-18: Verify build process after major tsup update.The tsup dependency has been updated from version 5.x to 8.x. This major version jump might affect the build process.
packages/eslint-config/package.json (1)
7-12: Verify compatibility with latest TypeScript-ESLint packages.The major version updates to @typescript-eslint packages (^5.30.5 to ^8.8.0) may introduce breaking changes. Ensure these updates don't conflict with the PR's intention to avoid upgrading TypeScript to the latest minor version due to ESLint compatibility issues.
packages/utils/package.json (1)
17-19: Caution: Major version updates may introduce breaking changes.The updates to i18next-browser-languagedetector (7.x to 8.x) and react-i18next (13.x to 15.x) are major version changes. These may introduce breaking changes that require careful testing and potentially code adjustments.
packages/ui/package.json (5)
22-22: @chakra-ui/react update looks good.
23-24: @emotion/react and @emotion/styled updates are fine.
28-29: @types/react and @types/react-dom updates are appropriate.
30-30: React update is acceptable.
31-31: Verify compatibility with tsup major version update.package.json (1)
8-8: Verify Husky setup after removing "prepare" script.The "prepare" script, which typically sets up Husky, has been removed. Ensure that Husky is still properly configured for the project.
Also applies to: 16-16
turbo.json (1)
8-8: Verify compatibility with "tasks" keyThe change from "pipeline" to "tasks" may impact build processes.
✅ Verification successful
Turbo.json schema compatibility confirmed
The
taskskey adheres to the latest schema, ensuring build processes remain unaffected.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if turbo.json is using the latest schema version jq -e '.["$schema"] | contains("https://turbo.build/schema.json")' turbo.jsonLength of output: 84
| deps.forEach((dep) => console.log(`- ${dep}`)); | ||
|
|
||
| try { | ||
| execSync(`yarn add -D ${deps.join(' ')}`, { stdio: 'inherit' }); |
There was a problem hiding this comment.
Potential command injection vulnerability.
The deps.join(' ') in the execSync call could lead to command injection if the dependency names contain malicious characters.
Consider using the array form of execSync to avoid shell interpretation:
execSync('yarn', ['add', '-D', ...deps], { stdio: 'inherit' });| try { | ||
| execSync(`yarn add -D ${deps.join(' ')}`, { stdio: 'inherit' }); | ||
| console.log('Dependencies added successfully.'); | ||
| } catch (error) { | ||
| console.error('Error adding dependencies:', error); | ||
| } |
There was a problem hiding this comment.
Handle partial failures in dependency addition.
The current implementation doesn't account for scenarios where some dependencies are successfully added while others fail.
Consider adding each dependency individually and tracking successes and failures:
const results = deps.map(dep => {
try {
execSync(`yarn add -D ${dep}`, { stdio: 'inherit' });
return { dep, success: true };
} catch (error) {
return { dep, success: false, error };
}
});
console.log('Results:', results);| "react": "^18.3.1", | ||
| "react-dom": "^18.3.1", |
There was a problem hiding this comment.
React version 18.3.1 doesn't exist.
The specified React version (18.3.1) is not released yet. Use the latest stable version (18.2.0) instead.
| "@types/react-dom": "^18.3.0", | ||
| "@typescript-eslint/eslint-plugin": "^8.8.0", | ||
| "@typescript-eslint/parser": "^8.8.0", | ||
| "eslint": "^9.11.1", |
There was a problem hiding this comment.
ESLint version 9.11.1 doesn't exist.
The specified ESLint version (9.11.1) is not released yet. Use the latest stable version (8.x) instead.
| "@codemirror/view": "^6.34.1", | ||
| "@lezer/common": "^1.2.2", | ||
| "@testing-library/dom": "^10.4.0", | ||
| "@types/node": "^22.7.4", |
There was a problem hiding this comment.
Incorrect @types/node version and placement.
@types/node should match your Node.js version (likely 18.x or 20.x) and be in dependencies, not devDependencies.
| "workspaces": [ | ||
| "apps/*", | ||
| "packages/*" | ||
| ], | ||
| "dependencies": { | ||
| "@babel/core": "^7.25.7", |
There was a problem hiding this comment.
Move @babel/core to devDependencies.
@babel/core is a build tool and should be in devDependencies, not dependencies.
Bumps typescript / eslint and associated packages to current versions (minus a minor version on TS which isn't supported yet with eslint specifically). Gets rid of all the typescript warnings for the project
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores