🔧 Migrate to ESLint v9 #3993
🔧 Migrate to ESLint v9 #3993
Conversation
actual-github-bot
bot
changed the title
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Bundle Stats — desktop-clientHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset No files were changed View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
Bundle Stats — loot-coreHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
matt-fidd
force-pushed
the
eslint-v9
branch
2 times, most recently
from
a49f9e2 to
5ac6db2
Compare
WalkthroughThis pull request introduces comprehensive changes to the project's ESLint configuration and related files. The modifications primarily involve updating the ESLint configuration from a traditional The update involves significant refactoring of ESLint-related files across different packages, including Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (3)
packages/desktop-client/src/browser-preload.browser.js (3)
Line range hint
39-57: Enhance service worker update handlingThe current implementation lacks error handling and user feedback for service worker updates.
Consider this enhancement:
let isUpdateReadyForDownload = false; let markUpdateReadyForDownload; const isUpdateReadyForDownloadPromise = new Promise(resolve => { markUpdateReadyForDownload = () => { isUpdateReadyForDownload = true; resolve(true); }; }); const updateSW = registerSW({ immediate: true, onNeedRefresh: markUpdateReadyForDownload, + onRegistered(registration) { + console.log('SW registered:', registration); + }, + onRegisterError(error) { + console.error('SW registration error:', error); + }, });
Line range hint
82-134: Enhance file handling securityThe current file handling implementation could benefit from additional security measures.
Consider these security enhancements:
openFileDialog: async ({ filters = [] }) => { return new Promise(resolve => { let createdElement = false; let input = document.body.querySelector( 'input[id="open-file-dialog-input"]', ); if (!input) { createdElement = true; input = document.createElement('input'); } input.type = 'file'; input.id = 'open-file-dialog-input'; input.value = null; const filter = filters.find(filter => filter.extensions); if (filter) { input.accept = filter.extensions.map(ext => '.' + ext).join(','); } + // Validate file size + const MAX_FILE_SIZE = 50 * 1024 * 1024; // 50MB input.onchange = e => { const file = e.target.files[0]; + if (file.size > MAX_FILE_SIZE) { + alert('File size exceeds maximum limit'); + return; + } const filename = file.name.replace(/.*(\.[^.]*)/, 'file$1'); if (file) { const reader = new FileReader(); reader.readAsArrayBuffer(file); reader.onload = async function (ev) { const filepath = `/uploads/${filename}`; + // Validate file type + const validTypes = new Set(['text/csv', 'application/json']); + if (!validTypes.has(file.type)) { + alert('Invalid file type'); + return; + } window.__actionsForMenu .uploadFile(filename, ev.target.result) .then(() => resolve([filepath])); };
Line range hint
169-173: Improve app update implementationThe current implementation of
applyAppUpdatecreates an unresolved promise.Fix the implementation:
applyAppUpdate: async () => { - updateSW(); - // Wait for the app to reload - await new Promise(() => {}); + try { + await updateSW(); + window.location.reload(); + } catch (error) { + console.error('Failed to apply update:', error); + throw error; + } },
🧹 Nitpick comments (14)
eslint.config.mjs (3)
680-680: Consider Re-enabling 'import/no-unused-modules' When StableThe rule
'import/no-unused-modules'is currently disabled due to crashing issues. Once the underlying issue is resolved, consider re-enabling it to help identify and remove unused modules, improving code cleanliness.
464-468: Plan to Re-enable Temporarily Disabled RulesThe following rules are currently disabled with a TODO comment to re-enable them:
'react/react-in-jsx-scope': 'off''no-var': 'warn''react/jsx-curly-brace-presence': 'warn''object-shorthand': ['warn', 'properties']Ensure to revisit and re-enable these rules to maintain code quality and consistency.
516-521: Review Disabled TypeScript Rules Post-MigrationThe TypeScript-specific rules are disabled during the migration:
'@typescript-eslint/no-var-requires': 'off''@typescript-eslint/no-empty-function': 'off''@typescript-eslint/no-require-imports': 'off'After completing the TypeScript migration, consider re-enabling these rules to enforce best practices in TypeScript code.
packages/desktop-client/src/build-shims.js (1)
11-12: Remove Redundant Export of 'process'Since the focus has shifted to exporting
global, the export ofprocessmay be redundant. Ifprocessis no longer used elsewhere, consider removing it to clean up the exports.Apply this diff to remove the redundant export:
export { process }; export { global };packages/loot-core/peg-transform.mjs (1)
9-9: Simplify Unused Variable SuppressionInstead of using an eslint-disable comment, you can prefix the unused parameter with an underscore to indicate it's intentionally unused, which suppresses the linter warning without additional comments.
Apply this diff:
-// eslint-disable-next-line @typescript-eslint/no-unused-vars process(sourceText, sourcePath, _options) {packages/api/index.ts (2)
11-12: Consider consolidating ESLint directivesMultiple ESLint disable directives could be combined. Also, the import resolution issues should be addressed in the ESLint config.
-// eslint-disable-next-line import/extensions, import/no-unresolved +/* eslint-disable import/extensions, import/no-unresolved */ import * as bundle from './app/bundle.api.js'; +/* eslint-enable import/extensions, import/no-unresolved */
Line range hint
19-21: Plan removal of deprecated exportThe comment indicates this export should be removed in v7. Consider:
- Creating a tracking issue for this removal
- Adding a proper deprecation warning using
console.warnWould you like me to create a GitHub issue to track the removal of this deprecated export?
packages/desktop-client/playwright.config.js (1)
Line range hint
1-1: Consider implementing a comprehensive module resolution strategyTo address the recurring module resolution issues across multiple files, consider:
- Implementing a consistent module alias system
- Configuring ESLint's import resolver plugins
- Aligning TypeScript and ESLint path configurations
Example configuration for the new ESLint flat config:
import { glob } from 'glob'; import path from 'path'; const tsConfig = glob.sync('**/tsconfig.json').map(file => ({ name: path.dirname(file), path: file })); export default [{ plugins: { '@typescript-eslint': tsPlugin, 'import': importPlugin }, settings: { 'import/resolver': { typescript: { project: tsConfig } } } }];packages/loot-core/src/types/server-handlers.d.ts (1)
21-21: Consider fixing the module resolution instead of disabling ESLint.While the ESLint disable directive works as a temporary solution, it would be better to properly configure the module resolution in your TypeScript and ESLint configuration. This ensures better maintainability and type safety.
Consider:
- Adding proper path aliases in
tsconfig.json- Configuring ESLint's import resolver to understand your project structure
- Using proper relative paths if the module is within the same package
-// eslint-disable-next-line import/no-unresolved -import { Query } from './query'; +import { Query } from '../shared/query';packages/loot-core/src/server/main.ts (1)
Line range hint
1-2412: Consider breaking down this large file for better maintainability.The file is quite large and handles multiple concerns. Consider splitting it into smaller, focused modules:
- Handler definitions
- Initialization logic
- Cloud storage operations
- Budget operations
This would improve:
- Code maintainability
- Testing capabilities
- Code reusability
- Error handling consistency
packages/desktop-client/vite.config.mts (4)
69-69: Consider removing empty line 69This empty line appears to be unnecessary and doesn't contribute to code readability.
Line range hint
71-82: Consider enhancing environment variable validationWhile the environment loading looks correct, consider adding validation for required environment variables to fail fast if critical configurations are missing.
Example enhancement:
export default defineConfig(async ({ mode }) => { const env = loadEnv(mode, process.cwd(), ''); + const requiredEnvVars = []; + const missingEnvVars = requiredEnvVars.filter(key => !env[key]); + if (missingEnvVars.length > 0) { + throw new Error(`Missing required environment variables: ${missingEnvVars.join(', ')}`); + } const devHeaders = { 'Cross-Origin-Opener-Policy': 'same-origin', 'Cross-Origin-Embedder-Policy': 'require-corp', };
Line range hint
143-157: Consider enhancing PWA caching strategiesThe current PWA configuration is basic. Consider adding more specific caching strategies for different types of assets to optimize performance.
Example enhancement:
VitePWA({ registerType: 'prompt', workbox: { globPatterns: [ '**/*.{js,css,html,txt,wasm,sql,sqlite,ico,png,woff2,webmanifest}', ], ignoreURLParametersMatching: [/^v$/], + runtimeCaching: [ + { + urlPattern: /^https:\/\/api\./i, + handler: 'NetworkFirst', + options: { + cacheName: 'api-cache', + expiration: { + maxEntries: 100, + maxAgeSeconds: 60 * 60 * 24, // 24 hours + }, + }, + }, + ], }, }),
Line range hint
108-134: Consider adding build optimization configurationsThe build configuration could benefit from additional optimizations for production builds.
Example enhancement:
build: { target: 'es2022', sourcemap: true, outDir: mode === 'desktop' ? 'build-electron' : 'build', assetsDir: 'static', manifest: true, assetsInlineLimit: 0, chunkSizeWarningLimit: 1500, + minify: 'terser', + terserOptions: { + compress: { + drop_console: mode === 'production', + drop_debugger: mode === 'production', + }, + }, rollupOptions: {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
upcoming-release-notes/3993.mdis excluded by!**/*.mdyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (25)
.eslintignore(0 hunks).eslintrc.js(0 hunks)eslint.config.mjs(1 hunks)package.json(1 hunks)packages/api/index.ts(1 hunks)packages/desktop-client/globals.d.ts(1 hunks)packages/desktop-client/playwright.config.js(1 hunks)packages/desktop-client/src/browser-preload.browser.js(1 hunks)packages/desktop-client/src/build-shims.js(1 hunks)packages/desktop-client/src/components/autocomplete/PayeeAutocomplete.tsx(0 hunks)packages/desktop-client/vite.config.mts(1 hunks)packages/desktop-electron/beforePackHook.ts(1 hunks)packages/eslint-plugin-actual/.eslintrc.js(0 hunks)packages/eslint-plugin-actual/lib/rules/__tests__/prefer-if-statement.js(0 hunks)packages/eslint-plugin-actual/lib/rules/__tests__/typography.js(0 hunks)packages/eslint-plugin-actual/lib/rules/prefer-if-statement.js(0 hunks)packages/eslint-plugin-actual/lib/rules/typography.js(0 hunks)packages/loot-core/peg-transform.mjs(1 hunks)packages/loot-core/src/platform/client/undo/index.d.ts(1 hunks)packages/loot-core/src/server/accounts/payees.ts(0 hunks)packages/loot-core/src/server/importers/ynab4-types.d.ts(0 hunks)packages/loot-core/src/server/importers/ynab5-types.d.ts(0 hunks)packages/loot-core/src/server/main.ts(1 hunks)packages/loot-core/src/shared/environment.ts(0 hunks)packages/loot-core/src/types/server-handlers.d.ts(1 hunks)
💤 Files with no reviewable changes (12)
- packages/loot-core/src/server/importers/ynab5-types.d.ts
- packages/loot-core/src/shared/environment.ts
- packages/eslint-plugin-actual/.eslintrc.js
- packages/eslint-plugin-actual/lib/rules/tests/typography.js
- packages/loot-core/src/server/importers/ynab4-types.d.ts
- .eslintignore
- packages/eslint-plugin-actual/lib/rules/tests/prefer-if-statement.js
- packages/eslint-plugin-actual/lib/rules/typography.js
- packages/eslint-plugin-actual/lib/rules/prefer-if-statement.js
- .eslintrc.js
- packages/loot-core/src/server/accounts/payees.ts
- packages/desktop-client/src/components/autocomplete/PayeeAutocomplete.tsx
✅ Files skipped from review due to trivial changes (1)
- packages/desktop-electron/beforePackHook.ts
🔇 Additional comments (9)
eslint.config.mjs (1)
525-541: Verify TypeScript Parser Configuration
Ensure that the TypeScript parser is correctly configured with the appropriate tsconfig.json paths. Incorrect paths may lead to unexpected linting behavior or rules not being applied properly.
Run the following script to check the existence of tsconfig.json:
✅ Verification successful
Let me verify the content of the tsconfig.json file to ensure it's properly configured for the ESLint setup.
TypeScript Configuration is Valid
The TypeScript configuration is correctly set up. The tsconfig.json exists at the root and contains appropriate compiler options for ESLint integration, including:
- Proper module resolution (
moduleResolution: "Node10") - JSX support (
jsx: "preserve") - Correct include/exclude patterns for the monorepo structure
- Essential compiler options for type checking
The path in the ESLint config (./tsconfig.json) correctly points to this file.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that tsconfig.json exists at the specified path.
# Test: Check if tsconfig.json exists. Expect: File exists.
CONFIG_PATH="$(pwd)/tsconfig.json"
if [ -f "$CONFIG_PATH" ]; then
echo "tsconfig.json found at $CONFIG_PATH"
else
echo "tsconfig.json not found at $CONFIG_PATH"
fi
Length of output: 220
Script:
#!/bin/bash
# Check the content of tsconfig.json and look for essential compiler options
cat tsconfig.json
# Also check for any other tsconfig files that might be present
fd tsconfig.json
Length of output: 1504
packages/desktop-client/globals.d.ts (1)
1-1:
Avoid Deep Import from '@emotion/css'
Importing CSSObject from an internal path can lead to module resolution errors. Instead, import directly from the package root to improve compatibility and prevent potential issues.
Apply this diff to adjust the import:
-import { type CSSObject } from '@emotion/css/dist/declarations/src/create-instance';
+import { type CSSObject } from '@emotion/css';Likely invalid or redundant comment.
packages/loot-core/src/platform/client/undo/index.d.ts (2)
Line range hint 4-30: LGTM! Well-structured type definitions
The type definitions are well-organized with:
- Clear type structure for
UndoState - Proper generic constraints for type safety
- Consistent type alias exports for function types
1-2: Consider fixing the module resolution for modals import
The ESLint disable directive is addressing a known issue mentioned in the PR objectives. Instead of disabling the rule, consider:
- Adding the path to
tsconfig.jsonpaths configuration - Using a module alias resolver in the ESLint config
✅ Verification successful
Import path is correctly configured, ESLint directive can be removed
The import path ../../client/state-types/modals is actually valid and properly configured in the project's TypeScript setup. The root tsconfig.json has a path alias configuration that maps loot-core/* to ./packages/loot-core/src/*, and the codebase consistently uses this pattern. The ESLint disable directive is unnecessary as the import resolution works correctly.
- Remove the ESLint disable directive from line 1
- The import can be kept as is, or optionally refactored to use the aliased path:
loot-core/src/client/state-types/modals
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the module path exists and is properly configured
fd -t f "modals.(ts|d.ts)" -x echo "Found potential module at: {}"
# Verify tsconfig paths configuration
fd -t f "tsconfig.*json" -x cat {} | jq -r '.compilerOptions.paths // empty'
Length of output: 523
Script:
#!/bin/bash
# Let's check the actual content of tsconfig files
fd -t f "tsconfig.*json" -x echo "=== Content of {}" \; -x cat {}
# Check the actual import usage in the codebase
rg -g "*.ts" -g "*.tsx" "from '.*modals'" -A 1
Length of output: 6971
packages/desktop-client/playwright.config.js (1)
66-66: LGTM! Properly scoped ESLint directive
The ESLint directive is correctly focused on the specific rule needed for the default export, which is a common pattern in configuration files.
package.json (2)
41-42: LGTM: Simplified lint command and added verbose mode
The simplified lint command aligns with ESLint v9's improved file extension handling. The addition of verbose mode will help debug configuration issues.
48-67: Verify compatibility between ESLint v9 and related plugins
The major version bump of ESLint (v8 to v9) along with updates to related packages requires careful verification:
- ESLint v9 with typescript-eslint v8
- ESLint v9 with eslint-plugin-react v7
- ESLint v9 with eslint-plugin-import v2
✅ Verification successful
The previous command had issues with the output being too large. Let's try a more focused approach to check ESLint configuration and verify the compatibility requirements.
Let's try to find the ESLint configuration to verify the compatibility requirements between ESLint v9 and the plugins.
Let's try one more time to locate any ESLint configuration files that might be hidden in subdirectories or have different naming patterns.
We found a reference to a root ESLint config file in the plugin's configuration. Let's check both the root config and the plugin's package.json to understand the version compatibility requirements.
ESLint v9 upgrade appears compatible with current plugin setup
The ESLint upgrade to v9 should be compatible with the current setup:
- The local
eslint-plugin-actualonly requireseslint: ">=7"in its peerDependencies, which is satisfied by v9 - The updated external plugins in package.json are all recent versions that support ESLint v9:
- typescript-eslint v8.x
- eslint-plugin-react v7.x
- eslint-plugin-import v2.x
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for known compatibility issues between ESLint v9 and plugins
# Check package.json files in the repository for potential conflicts
echo "Checking for potential plugin conflicts..."
rg -l '"eslint":|"@typescript-eslint|"eslint-plugin' | xargs cat
Length of output: 69143
Script:
#!/bin/bash
# Check for ESLint configuration files to verify plugin compatibility
echo "Checking ESLint configuration files..."
fd -e js -e json -e yaml -e yml -g '*eslint*' --exec cat {}
# Check for any TypeScript configuration that might affect ESLint
echo -e "\nChecking TypeScript configuration..."
fd -g 'tsconfig*.json' --exec cat {}
Length of output: 3365
Script:
#!/bin/bash
# Let's check for the actual ESLint configuration file at the root
echo "Checking root .eslintrc file..."
cat .eslintrc.js || cat .eslintrc.json || cat .eslintrc.yaml || cat .eslintrc.yml || cat .eslintrc
# Also check for any ESLint extends or plugin configurations in package.json
echo -e "\nChecking package.json for ESLint configuration..."
fd -g 'package.json' -x jq -r '.eslintConfig' {} 2>/dev/null
Length of output: 595
Script:
#!/bin/bash
# Search for any ESLint config files in the entire repository
echo "Searching for ESLint config files recursively..."
fd -H -t f -e js -e json -e yaml -e yml -g '*.eslint*' -x echo "Found: {}" \; -x cat {}
# Check for any ESLint related files in the root config directory
echo -e "\nChecking config directory..."
fd -t f -g '*eslint*' config/ -x echo "Found: {}" \; -x cat {} 2>/dev/null
Length of output: 790
Script:
#!/bin/bash
# The eslint plugin config extends from root config, let's check for it
echo "Checking for root ESLint config..."
cat ./.eslintrc.js
# Let's also check the actual version compatibility requirements in the eslint plugin's package.json
echo -e "\nChecking ESLint plugin package.json..."
cat ./packages/eslint-plugin-actual/package.json
Length of output: 730
packages/loot-core/src/server/main.ts (1)
Line range hint 2395-2412: Great improvement in type safety! 👍
The addition of proper TypeScript generics to the send function improves type checking and provides better IDE support. The implementation correctly preserves the handler's parameter and return types.
packages/desktop-client/src/browser-preload.browser.js (1)
1-3: Address ESLint module resolution warning
The PR objectives mention an unresolved module error for 'virtual:pwa-register'. This needs to be addressed in the ESLint configuration.
Let's verify the module resolution configuration:
|
I'm having some trouble getting the tests for |
|
This is definitely a very welcome change and I don't mean to distract away from it, but.. did you consider swapping over to biomejs? Allegedly it's a MUCH faster solution to linting. So if we could replace eslint & prettier with biome - we could run it always in a pre-commit hook. But swapping to it might be a bigger effort, so I don't want it to be a blocker here.. :) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/eslint-plugin-actual/lib/rules/typography.test.js (1)
1-2: Consider removing or justifying the inline rule disable comment.
Disabling therulesdir/typographyrule at the top of the file might reduce coverage of scenarios where the rule is tested or verified. If this is intentional for these tests, leave a comment explaining why.packages/eslint-plugin-actual/lib/rules/prefer-if-statement.test.js (2)
1-2: Clarify the purpose of disabling typography rules.It may be helpful to comment on why
rulesdir/typographyis disabled. If typography rules are relevant to code style or readability, consider enabling them or marking TODOs for re-enabling.
9-9: Consider using a named import for clarity.Using a named import can help clarify which parts of the rule API are being tested, especially if this file grows larger.
-import * as rule from './prefer-if-statement'; +import { rule } from './prefer-if-statement';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
packages/eslint-plugin-actual/lib/rules/__tests__/prefer-if-statement.js(0 hunks)packages/eslint-plugin-actual/lib/rules/__tests__/typography.js(0 hunks)packages/eslint-plugin-actual/lib/rules/prefer-if-statement.test.js(1 hunks)packages/eslint-plugin-actual/lib/rules/typography.test.js(1 hunks)packages/eslint-plugin-actual/package.json(1 hunks)
💤 Files with no reviewable changes (2)
- packages/eslint-plugin-actual/lib/rules/tests/typography.js
- packages/eslint-plugin-actual/lib/rules/tests/prefer-if-statement.js
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: compare
🔇 Additional comments (11)
packages/eslint-plugin-actual/lib/rules/typography.test.js (4)
7-9: Imports look good!
The usage ofrunClassicfromeslint-vitest-rule-testerand loading the localtypographyrule is correct for a Vitest-based test suite.
15-27: Valid test cases are comprehensive.
They cover different string usages, including single quotes, double quotes, template literals, selectors, and methods. This ensures that the rule is validated against varied real-world scenarios.
29-65: Invalid test cases are thorough.
Each scenario correctly includes multiple errors to validate rule detection and messages. The usage of unique test codes ensures coverage of template strings, function parameters, and JSX text.
67-73: Parser options are well-defined.
ProvidingecmaVersion: 2020and enablingjsxis essential for covering modern JS and React-specific cases in this rule.packages/eslint-plugin-actual/package.json (2)
16-17: New dependencies align with Vitest testing environment.
This transition replaces Jest with Vitest, enabling usage of theeslint-vitest-rule-testerlibrary for rule testing. Good job maintaining consistency in dev dependencies.
8-8: Ensure all references to the old test command have been removed.
After switching to"test": "vitest", verify that references tojestscripts or configs don't remain elsewhere in the workspace.packages/eslint-plugin-actual/lib/rules/prefer-if-statement.test.js (5)
7-8: Confirm that 'eslint-vitest-rule-tester' is the intended dependency.If your test environment is already using Vitest, confirm that this library and version align with your current setup to avoid potential compatibility issues.
15-28: Good coverage of valid scenarios.The test suite includes a wide variety of valid cases (logical operators, ternaries, JSX), indicating thorough coverage. No issues found.
30-58: Thorough invalid scenario coverage.The invalid test inputs cover multiple operator types. This is essential in confirming that the rule flags all cases of logical and ternary statements outside of
ifblocks.
86-87: Double-check the semantics of optional chaining vs. logical operators.Rewriting
foo && foo();tofoo?.();changes the condition from “truthy check” to “not null or undefined.” Values such as0,'', orfalseare treated differently. Verify that this is the correct and desired transformation for your codebase.
137-140: Confirm correct parser options alignment.ECMAScript 2020 plus JSX support seems aligned with the test scenarios. Ensure these parser options match the wider ESLint configuration for consistent linting behavior across the project.
|
Pushed the test patch. Hope you don't mind. |
Not at all, thank you |
I didn't, but now I've got something to look into! Let's get this in after the freeze (lint running times at the moment are doing my head in) and I'll take a look at biome when I get a chance. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/desktop-client/src/build-shims.js (1)
12-12: Consider combining exports for better maintainability.Instead of having separate export statements, consider combining them into a single export statement for better maintainability.
-export { process }; - -export { global }; +export { process, global };eslint.config.mjs (2)
19-25: Consider using path.resolve for cross-platform compatibility.The current path joining might cause issues on different platforms. Using
path.resolvewould be more robust.-pluginRulesDir.RULES_DIR = path.join( - __dirname, - 'packages', - 'eslint-plugin-actual', - 'lib', - 'rules', -); +pluginRulesDir.RULES_DIR = path.resolve( + __dirname, + 'packages', + 'eslint-plugin-actual', + 'lib', + 'rules' +);
463-468: Consider grouping related rules together.The TODO comment and related rules could be grouped together in a separate configuration object for better organization and future maintenance.
- // TODO: re-enable these rules - 'react/react-in-jsx-scope': 'off', - 'no-var': 'warn', - 'react/jsx-curly-brace-presence': 'warn', - 'object-shorthand': ['warn', 'properties'], + // Rules temporarily disabled or with reduced severity + ...{ + // TODO: re-enable these rules with stricter settings + 'react/react-in-jsx-scope': 'off', + 'no-var': 'warn', + 'react/jsx-curly-brace-presence': 'warn', + 'object-shorthand': ['warn', 'properties'], + },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
upcoming-release-notes/3993.mdis excluded by!**/*.mdyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (28)
.eslintignore(0 hunks).eslintrc.js(0 hunks)eslint.config.mjs(1 hunks)package.json(1 hunks)packages/api/index.ts(1 hunks)packages/desktop-client/globals.d.ts(1 hunks)packages/desktop-client/playwright.config.js(1 hunks)packages/desktop-client/src/browser-preload.browser.js(1 hunks)packages/desktop-client/src/build-shims.js(1 hunks)packages/desktop-client/src/components/autocomplete/PayeeAutocomplete.tsx(0 hunks)packages/desktop-client/vite.config.mts(1 hunks)packages/desktop-electron/beforePackHook.ts(1 hunks)packages/eslint-plugin-actual/.eslintrc.js(0 hunks)packages/eslint-plugin-actual/lib/rules/__tests__/prefer-if-statement.js(0 hunks)packages/eslint-plugin-actual/lib/rules/__tests__/typography.js(0 hunks)packages/eslint-plugin-actual/lib/rules/prefer-if-statement.js(0 hunks)packages/eslint-plugin-actual/lib/rules/prefer-if-statement.test.js(1 hunks)packages/eslint-plugin-actual/lib/rules/typography.js(0 hunks)packages/eslint-plugin-actual/lib/rules/typography.test.js(1 hunks)packages/eslint-plugin-actual/package.json(1 hunks)packages/loot-core/peg-transform.mjs(1 hunks)packages/loot-core/src/platform/client/undo/index.d.ts(1 hunks)packages/loot-core/src/server/accounts/payees.ts(0 hunks)packages/loot-core/src/server/importers/ynab4-types.d.ts(0 hunks)packages/loot-core/src/server/importers/ynab5-types.d.ts(0 hunks)packages/loot-core/src/server/main.ts(1 hunks)packages/loot-core/src/shared/environment.ts(0 hunks)packages/loot-core/src/types/server-handlers.d.ts(1 hunks)
💤 Files with no reviewable changes (12)
- packages/loot-core/src/shared/environment.ts
- packages/loot-core/src/server/importers/ynab5-types.d.ts
- packages/eslint-plugin-actual/lib/rules/tests/typography.js
- packages/eslint-plugin-actual/.eslintrc.js
- .eslintignore
- packages/loot-core/src/server/importers/ynab4-types.d.ts
- packages/eslint-plugin-actual/lib/rules/tests/prefer-if-statement.js
- packages/eslint-plugin-actual/lib/rules/typography.js
- packages/eslint-plugin-actual/lib/rules/prefer-if-statement.js
- packages/loot-core/src/server/accounts/payees.ts
- .eslintrc.js
- packages/desktop-client/src/components/autocomplete/PayeeAutocomplete.tsx
🚧 Files skipped from review as they are similar to previous changes (13)
- packages/desktop-client/globals.d.ts
- packages/loot-core/peg-transform.mjs
- packages/eslint-plugin-actual/lib/rules/typography.test.js
- packages/loot-core/src/types/server-handlers.d.ts
- packages/eslint-plugin-actual/lib/rules/prefer-if-statement.test.js
- packages/desktop-client/vite.config.mts
- packages/eslint-plugin-actual/package.json
- packages/desktop-electron/beforePackHook.ts
- packages/api/index.ts
- packages/desktop-client/playwright.config.js
- packages/desktop-client/src/browser-preload.browser.js
- packages/loot-core/src/platform/client/undo/index.d.ts
- package.json
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: typecheck
- GitHub Check: build (macos-latest)
- GitHub Check: web
- GitHub Check: build (windows-latest)
- GitHub Check: crdt
- GitHub Check: test
- GitHub Check: api
- GitHub Check: build (ubuntu-latest)
- GitHub Check: Analyze
- GitHub Check: Wait for Netlify build to finish
- GitHub Check: compare
🔇 Additional comments (2)
eslint.config.mjs (1)
532-532: Verify the TypeScript configuration file path.The hardcoded path to
tsconfig.jsonmight not work in all project structures. Consider making this path configurable or using path resolution.✅ Verification successful
Let me verify the content of the root tsconfig.json and check if it's referenced by other configuration files.
The hardcoded path to
tsconfig.jsonis correct and well-referencedThe path
./tsconfig.jsonin the ESLint configuration is correct because:
- The root
tsconfig.jsonexists and is properly configured as the base TypeScript configuration- Multiple package-specific TypeScript configurations extend from this root config using relative paths (../../tsconfig.json)
- The root config properly sets up the monorepo structure with appropriate includes, excludes, and module resolution settings
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if tsconfig.json exists at the specified location # and look for any other TypeScript configuration files that might be relevant. echo "Checking for TypeScript configuration files..." fd -e json -e jsonc '^tsconfig' --exec echo "Found TypeScript config: {}"Length of output: 474
Script:
#!/bin/bash # Check the content of root tsconfig.json and look for references to it echo "Content of root tsconfig.json:" cat tsconfig.json echo -e "\nChecking for references to root tsconfig.json in other TS configs:" rg "extends.*tsconfig.json" -g "tsconfig*.json"Length of output: 2034
packages/loot-core/src/server/main.ts (1)
Line range hint
2559-2571: Excellent type-safe implementation of the lib export.The implementation provides strong type safety for message handling with:
- Generic type constraints for handler names and arguments
- Proper TypeScript type inference for async operations
- Clear interface for external consumption
However, consider adding JSDoc comments to document the exported functions and their usage:
export const lib = { + /** Get the data directory path */ getDataDir: fs.getDataDir, + /** Send a message through the connection */ sendMessage: (msg, args) => connection.send(msg, args), + /** + * Send a handler message with type-safe arguments and return value + * @template K - Handler name + * @template T - Handler function type + */ send: async <K extends keyof Handlers, T extends Handlers[K]>( name: K, args?: Parameters<T>[0], ): Promise<Awaited<ReturnType<T>>> => { const res = await runHandler(app.handlers[name], args); return res; }, + /** Register an event listener */ on: (name, func) => app.events.on(name, func), q, db, };
| 'import/no-default-export': 'off', | ||
| }, | ||
| }, | ||
| {}, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove empty configuration object.
This empty configuration object serves no purpose and should be removed.
- {},
matt-fidd
force-pushed
the
eslint-v9
branch
4 times, most recently
from
fa54905 to
86ab57b
Compare
|
I'm still struggling with these tests, not having my laptop isn't helpful either... |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
packages/eslint-plugin-actual/lib/rules/__tests__/typography.test.js (2)
19-27: Consider adding more edge cases to the test suite.The current valid test cases cover common scenarios well. Consider adding these edge cases to make the test suite more robust:
- Unicode quotes (e.g., curly quotes)
- Multi-line strings
- Empty strings
- Strings with escaped quotes
Example additions:
valid: [ // ... existing cases ... + `let empty = '';`, + `let escaped = 'I said \\'Hello\\' world';`, + `let multiline = 'Line 1\nLine 2';`, + `let unicode = '‟Smart quotes"';`, ],
29-72: Add tests for automatic fix suggestions.The invalid test cases thoroughly check error detection. However, they don't verify that the rule can automatically fix the issues. Consider adding
outputfields to test the auto-fix functionality:invalid: [ { code: 'var test = `I said "Hello, world!"`;', + output: 'var test = \'I said "Hello, world!"\';', errors: [ { messageId: 'quote', type: 'TemplateElement', column: 20 }, { messageId: 'quote', type: 'TemplateElement', column: 34 }, ], }, // Add output field to other test cases... ],The parser options look good and appropriately support the test cases.
packages/eslint-plugin-actual/lib/rules/__tests__/prefer-if-statement.test.js (4)
1-1: Remove unnecessary ESLint disable comment.The
eslint-disable rulesdir/typographycomment appears to be unnecessary as there are no typography-related issues in this test file.-/* eslint-disable rulesdir/typography */
19-28: Add more test cases for better coverage.Consider adding these test cases to improve coverage:
- Nested conditions:
if (foo && (bar || baz))- Complex expressions:
if ((foo > 0) && (bar === 'test'))- Optional chaining:
if (foo?.bar?.baz)valid: [ `if (foo && bar) { console.log('Hello, world!'); }`, `myFunc(foo && bar);`, `if (foo || bar) { console.log('Hello, world!'); }`, `myFunc(foo || bar);`, `if (foo ? bar : baz) { console.log('Hello, world!'); }`, `<div>{foo && bar}</div>`, `<div>{foo || bar}</div>`, `<div>{foo ? bar : baz}</div>`, + `if (foo && (bar || baz)) { console.log('nested'); }`, + `if ((foo > 0) && (bar === 'test')) { console.log('complex'); }`, + `if (foo?.bar?.baz) { console.log('optional chaining'); }`, ],
136-141: Update parser configuration for ESLint v9.Since this PR migrates to ESLint v9, consider:
- Updating
ecmaVersionto 'latest' for future compatibility- Adding
sourceType: 'module'for explicit module support{ parserOptions: { - ecmaVersion: 2020, + ecmaVersion: 'latest', + sourceType: 'module', ecmaFeatures: { jsx: true }, }, },
7-9: Consider adding TypeScript-specific test cases.Since this is part of a larger ESLint v9 migration, it would be beneficial to add test cases that verify the rule's behavior with TypeScript-specific syntax (e.g., type assertions, generics).
This would require:
- Adding
@typescript-eslint/parserto the test configuration- Including TypeScript-specific test cases
+import { ParserOptions } from '@typescript-eslint/parser'; import { runClassic } from 'eslint-vitest-rule-tester'; import * as rule from '../prefer-if-statement'; +// Add TypeScript test cases +const tsValidExamples = [ + 'if ((foo as any) && (bar as string)) {}', + 'if (foo as unknown as boolean) {}', + 'myFunc<T>(foo && bar);', +];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/eslint-plugin-actual/.eslintrc.js(0 hunks)packages/eslint-plugin-actual/lib/rules/__tests__/prefer-if-statement.test.js(1 hunks)packages/eslint-plugin-actual/lib/rules/__tests__/typography.test.js(1 hunks)
💤 Files with no reviewable changes (1)
- packages/eslint-plugin-actual/.eslintrc.js
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build (macos-latest)
- GitHub Check: Analyze
- GitHub Check: build (windows-latest)
- GitHub Check: Wait for Netlify build to finish
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (2)
packages/eslint-plugin-actual/lib/rules/__tests__/typography.test.js (1)
1-10: LGTM! Clean setup with proper imports.The migration to Vitest and the rule setup look good. The rule disable is appropriately placed to prevent self-linting.
packages/eslint-plugin-actual/lib/rules/__tests__/prefer-if-statement.test.js (1)
30-134: Review inconsistent autofix behavior.The rule suggests optional chaining fixes for some cases but not others. For example:
foo && foo()→foo?.()foo.bar && foo.bar()→foo.bar?.()- But similar expressions like standalone logical operators have no suggested fixes.
Consider standardizing the autofix behavior across similar patterns.
|
There you go |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/eslint-plugin-actual/lib/rules/__tests__/prefer-if-statement.test.js (5)
17-26: Consider adding more edge cases to valid test suite.The valid test cases are good but could be more comprehensive. Consider adding:
- Template literals:
`${foo && bar}`- Array/object methods:
array.filter(item => item && item.valid)- Async patterns:
async () => await foo && barvalid: [ `if (foo && bar) { console.log('Hello, world!'); }`, `myFunc(foo && bar);`, `if (foo || bar) { console.log('Hello, world!'); }`, `myFunc(foo || bar);`, `if (foo ? bar : baz) { console.log('Hello, world!'); }`, `<div>{foo && bar}</div>`, `<div>{foo || bar}</div>`, `<div>{foo ? bar : baz}</div>`, + // Template literals + `\`\${foo && bar}\``, + // Array/object methods + `array.filter(item => item && item.valid)`, + // Async patterns + `async () => await foo && bar`, ],
28-82: Enhance error messages and test coverage.While the test cases cover basic scenarios, consider these improvements:
- Make error messages more descriptive by including suggested alternatives
- Add test cases for common patterns like null coalescing
errors: [ { messageId: 'logical', data: { op: '&&' }, + // Add suggestion in the message + data: { + op: '&&', + suggestion: 'Use an if statement instead of standalone logical expression' + }, type: 'ExpressionStatement', }, ], // Add test case for null coalescing +{ + code: 'foo ?? bar;', + output: null, + errors: [ + { + messageId: 'logical', + data: { op: '??' }, + type: 'ExpressionStatement', + }, + ], +},
83-126: Add more optional chaining test cases.The optional chaining tests are good but could be enhanced with:
- Nested optional chaining scenarios
- Array access patterns
// Add after line 126 +{ + code: 'foo.bar && foo.bar.baz.qux();', + output: 'foo.bar?.baz?.qux();', + errors: [ + { + messageId: 'logical', + data: { op: '&&' }, + type: 'ExpressionStatement', + }, + ], +}, +{ + code: 'arr[0] && arr[0].foo();', + output: 'arr[0]?.foo();', + errors: [ + { + messageId: 'logical', + data: { op: '&&' }, + type: 'ExpressionStatement', + }, + ], +},
127-132: Add more complex ternary test cases.Consider adding test cases for:
- Nested ternary expressions
- Ternaries with complex function calls
// Add after line 131 +{ + code: 'foo ? (bar ? baz() : qux()) : quux();', + output: null, + errors: [ + { messageId: 'ternary', type: 'ExpressionStatement' }, + { messageId: 'ternary', type: 'ExpressionStatement' } + ], +}, +{ + code: 'foo ? async () => await bar() : baz();', + output: null, + errors: [{ messageId: 'ternary', type: 'ExpressionStatement' }], +},
134-139: Consider updating ECMAScript version.Since the codebase is being modernized (as evidenced by the PR objectives mentioning ESLint v9 migration), consider updating to a more recent ECMAScript version.
parserOptions: { - ecmaVersion: 2020, + ecmaVersion: 2024, ecmaFeatures: { jsx: true }, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/api/index.ts(1 hunks)packages/eslint-plugin-actual/lib/rules/__tests__/prefer-if-statement.test.js(1 hunks)packages/eslint-plugin-actual/lib/rules/__tests__/typography.test.js(1 hunks)packages/loot-core/src/server/tools/types/handlers.d.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/loot-core/src/server/tools/types/handlers.d.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/eslint-plugin-actual/lib/rules/tests/typography.test.js
- packages/api/index.ts
🔇 Additional comments (1)
packages/eslint-plugin-actual/lib/rules/__tests__/prefer-if-statement.test.js (1)
1-12: LGTM! Clean test setup.The test setup follows ESLint plugin testing best practices with clear imports and structure.
Amazing, thank you |
This is quite a big change diff wise, it took /far/ too long to get the new flat config format to work.
Unfortunately, I can not get the below rule to work. When enabled, ESLint crashes completely, with very little in the way of useful logging.
I also fixed some strict usage that it was complaining about in the custom rule package.
These errors are new in this new config. I've ignored them, but if you can see better fixes please shout.