Re-enable linting on the server #4363
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. |
matt-fidd
force-pushed
the
lint-server
branch
from
82a6e86 to
8eb7fcd
Compare
matt-fidd
force-pushed
the
lint-server
branch
from
8eb7fcd to
0f4646b
Compare
matt-fidd
force-pushed
the
lint-server
branch
from
0f4646b to
136a1b9
Compare
matt-fidd
changed the title
WalkthroughThis pull request updates various aspects of the codebase primarily in the Suggested labels
Suggested reviewers
✨ Finishing Touches
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: 1
🔭 Outside diff range comments (1)
packages/sync-server/src/app-sync.test.js (1)
849-854:⚠️ Potential issueFix potential SQL injection vulnerability in
addMockFile.The SQL query in
addMockFileuses string concatenation for the query, which could lead to SQL injection. Although this is a test file, it's good practice to use parameterized queries consistently.- 'INSERT INTO files (id, group_id, encrypt_keyid, encrypt_meta, sync_version, owner) VALUES (?, ?, ?,?, ?, ?)', + 'INSERT INTO files (id, group_id, encrypt_keyid, encrypt_meta, sync_version, owner) VALUES (?, ?, ?, ?, ?, ?)',
🧹 Nitpick comments (4)
packages/sync-server/app.js (1)
5-6: Fix typo in comment.There's a typo in the comment: "becasue" should be "because".
- //import the app here becasue initial migrations need to be run first - they are dependencies of the app.js + //import the app here because initial migrations need to be run first - they are dependencies of the app.jseslint.config.mjs (1)
838-854: Consider adding documentation for the disabled rules.The changes appropriately re-enable linting for the
sync-serverpackage with necessary exceptions. Consider adding comments explaining why specific rules are disabled, particularly forapp-gocardless/banks/*.jsfiles.Add comments explaining:
- Why
import/extensionsandrulesdir/typographyare disabled for allsync-serverfiles- Why
import/no-anonymous-default-export,import/no-default-export, and@typescript-eslint/no-unused-varsare disabled forapp-gocardless/banks/*.jsfilespackages/sync-server/src/app-sync.test.js (2)
663-664: Improve test description consistency.The comment above the test case is redundant with the test description. Consider removing the comment to maintain DRY principles.
- // it returns 422 if the fileId is not provided it('returns 422 if the fileId is not provided', async () => {
254-258: Enhance assertion messages for better test failure debugging.Consider adding more descriptive error messages to assertions to make test failures more informative.
- expect(rowsAfter.length).toBe(1); - expect(rowsAfter[0].id).toEqual(fileId); - expect(rowsAfter[0].group_id).toEqual(receivedGroupid); - expect(rowsAfter[0].sync_version).toEqual(syncVersion); - expect(rowsAfter[0].name).toEqual(fileName); + expect(rowsAfter.length).toBe(1, 'Expected exactly one file record'); + expect(rowsAfter[0].id).toEqual(fileId, 'File ID mismatch'); + expect(rowsAfter[0].group_id).toEqual(receivedGroupid, 'Group ID mismatch'); + expect(rowsAfter[0].sync_version).toEqual(syncVersion, 'Sync version mismatch'); + expect(rowsAfter[0].name).toEqual(fileName, 'File name mismatch');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4363.mdis excluded by!**/*.md
📒 Files selected for processing (40)
eslint.config.mjs(2 hunks)packages/sync-server/app.js(1 hunks)packages/sync-server/jest.global-setup.js(2 hunks)packages/sync-server/jest.global-teardown.js(1 hunks)packages/sync-server/migrations/1694360000000-create-folders.js(1 hunks)packages/sync-server/migrations/1694360479680-create-account-db.js(1 hunks)packages/sync-server/migrations/1694362247011-create-secret-table.js(1 hunks)packages/sync-server/migrations/1702667624000-rename-nordigen-secrets.js(1 hunks)packages/sync-server/migrations/1718889148000-openid.js(1 hunks)packages/sync-server/migrations/1719409568000-multiuser.js(5 hunks)packages/sync-server/src/account-db.js(2 hunks)packages/sync-server/src/accounts/openid.js(3 hunks)packages/sync-server/src/accounts/password.js(3 hunks)packages/sync-server/src/app-account.js(2 hunks)packages/sync-server/src/app-admin.js(3 hunks)packages/sync-server/src/app-admin.test.js(1 hunks)packages/sync-server/src/app-gocardless/bank-factory.js(1 hunks)packages/sync-server/src/app-gocardless/banks/easybank_bawaatww.js(1 hunks)packages/sync-server/src/app-gocardless/banks/hype_hyeeit22.js(3 hunks)packages/sync-server/src/app-gocardless/services/gocardless-service.js(2 hunks)packages/sync-server/src/app-gocardless/tests/bank-factory.spec.js(1 hunks)packages/sync-server/src/app-secrets.js(1 hunks)packages/sync-server/src/app-simplefin/app-simplefin.js(2 hunks)packages/sync-server/src/app-sync.js(3 hunks)packages/sync-server/src/app-sync.test.js(1 hunks)packages/sync-server/src/app-sync/services/files-service.js(4 hunks)packages/sync-server/src/app-sync/tests/services/files-service.test.js(1 hunks)packages/sync-server/src/app.js(2 hunks)packages/sync-server/src/db.js(1 hunks)packages/sync-server/src/load-config.js(2 hunks)packages/sync-server/src/migrations.js(1 hunks)packages/sync-server/src/run-migrations.js(1 hunks)packages/sync-server/src/scripts/enable-openid.js(2 hunks)packages/sync-server/src/scripts/health-check.js(1 hunks)packages/sync-server/src/services/secrets-service.js(1 hunks)packages/sync-server/src/services/user-service.js(1 hunks)packages/sync-server/src/sync-simple.js(1 hunks)packages/sync-server/src/util/middlewares.js(1 hunks)packages/sync-server/src/util/paths.js(1 hunks)packages/sync-server/src/util/validate-user.js(3 hunks)
✅ Files skipped from review due to trivial changes (7)
- packages/sync-server/src/load-config.js
- packages/sync-server/src/services/user-service.js
- packages/sync-server/src/app-admin.test.js
- packages/sync-server/src/app-gocardless/tests/bank-factory.spec.js
- packages/sync-server/src/app-secrets.js
- packages/sync-server/migrations/1702667624000-rename-nordigen-secrets.js
- packages/sync-server/src/sync-simple.js
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Analyze
- GitHub Check: build (macos-latest)
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: Build Docker image (alpine)
- GitHub Check: Build Docker image (ubuntu)
🔇 Additional comments (40)
packages/sync-server/src/run-migrations.js (1)
1-1: LGTM!The change from default import to named import aligns with the PR objective and maintains consistency across the codebase.
packages/sync-server/jest.global-teardown.js (1)
1-3: LGTM!The change to named import with alias and the ESLint disable comment are appropriate for maintaining the default export while aligning with the linting objectives.
packages/sync-server/migrations/1694362247011-create-secret-table.js (1)
1-1: LGTM!The change from default import to named import aligns with the PR objective and maintains consistency across the codebase.
packages/sync-server/src/util/paths.js (1)
3-3: LGTM!The change from default import to named import aligns with the PR objective and maintains consistency across the codebase.
packages/sync-server/app.js (1)
1-1: LGTM!The change from default import to named import aligns with the PR objective to re-enable linting.
packages/sync-server/migrations/1694360000000-create-folders.js (2)
3-3: LGTM!The change from default import to named import aligns with the PR objective to re-enable linting.
9-9: LGTM!The change from loose equality (
==) to strict equality (===) improves type safety.packages/sync-server/src/scripts/health-check.js (1)
3-3: LGTM!The change from default import to named import aligns with the PR objective to re-enable linting.
packages/sync-server/migrations/1694360479680-create-account-db.js (1)
1-1: LGTM!The change from default import to named import aligns with the PR objective to re-enable linting.
packages/sync-server/src/migrations.js (1)
5-5: Verify import/export changes across the codebase.The changes from default to named imports/exports look good and align with the PR objective. Let's verify that these changes are consistently applied across the codebase.
Also applies to: 7-7
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify that all imports of `config` and `run` are using named imports. # Test: Search for imports of `config` from load-config.js echo "Checking config imports:" rg -A 1 "import.*from.*load-config" # Test: Search for imports of `run` from migrations.js echo "Checking run imports:" rg -A 1 "import.*from.*migrations"Length of output: 3158
Consistent Named Imports Across Codebase
The search results confirm that the migration to named imports/exports is applied consistently. All references to
configfromload-config.jsandrunfrommigrations.jsin the codebase now use named imports as expected.
configis consistently imported in files such aspackages/sync-server/src/app.js,packages/sync-server/src/accounts/openid.js, etc.runis imported via a named import (e.g.,import { run as runMigrations } from './src/migrations.js';inpackages/sync-server/app.jsand corresponding test/setup files).This verifies that the export/import changes from default to named are thoroughly and correctly implemented.
packages/sync-server/migrations/1718889148000-openid.js (1)
1-1: Verify import changes across the codebase.The change from default to named import looks good and aligns with the PR objective. Let's verify that this change is consistently applied across the codebase.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify that all imports of `getAccountDb` are using named imports. # Test: Search for imports of `getAccountDb` from account-db.js echo "Checking getAccountDb imports:" rg -A 1 "import.*from.*account-db"Length of output: 3983
Import Changes Verified for
getAccountDbAcross the CodebaseThe search confirms that all instances importing from
account-db.jsare using named imports (e.g.,import { getAccountDb } from '...';), consistent with the PR objective.
- Example files include:
packages/sync-server/jest.global-setup.js- Several migration files (e.g.,
1718889148000-openid.js,1694360479680-create-account-db.js)- Other modules like
packages/sync-server/src/accounts/password.jsandpackages/sync-server/src/services/user-service.jsThe change from default to named import is consistently applied across the codebase.
packages/sync-server/src/db.js (1)
56-56: Verify export changes across the codebase.The change from default to named export looks good and aligns with the PR objective. Let's verify that this change is consistently applied across the codebase.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify that all imports of `openDatabase` are using named imports. # Test: Search for imports of `openDatabase` from db.js echo "Checking openDatabase imports:" rg -A 1 "import.*from.*db\\.js"Length of output: 4309
Export change for openDatabase is applied consistently across the codebase.
- Verified direct imports (e.g., in
packages/sync-server/src/account-db.jsandpackages/sync-server/src/sync-simple.js) correctly use the named import{ openDatabase }from./db.js.- No instances of default import syntax for openDatabase were found.
packages/sync-server/src/util/validate-user.js (2)
4-4: LGTM! Named imports/exports improve code clarity.The transition from default to named imports/exports makes the code more explicit and helps with tree-shaking.
Also applies to: 13-13
60-60: LGTM! Strict equality improves type safety.Using
===instead of==ensures both value and type are checked, preventing potential type coercion issues.packages/sync-server/src/app-gocardless/banks/easybank_bawaatww.js (1)
20-20: LGTM! Strict inequality improves type safety in sorting.Using
!==instead of!=ensures both value and type are checked, which is particularly important in sorting functions where type consistency affects the sort order.packages/sync-server/src/util/middlewares.js (1)
4-4: LGTM! Named import matches export in validate-user.js.The change to use named import aligns with the corresponding export change in validate-user.js, maintaining consistency.
packages/sync-server/src/app-gocardless/bank-factory.js (1)
27-31: LGTM! Named function export improves code clarity.Converting the anonymous function to a named export
BankFactoryimproves:
- Code clarity and readability
- Stack traces and debugging
- Consistency with the codebase's move towards named exports
packages/sync-server/src/services/secrets-service.js (1)
3-3: LGTM!The change from default to named import aligns with the PR's objective to standardize imports across the codebase.
packages/sync-server/src/app.js (1)
16-16: LGTM!The changes to import and export statements align with the PR's objective to standardize module interfaces. The file maintains robust security configurations including rate limiting, CORS, and proper HTTPS setup.
Also applies to: 81-81
packages/sync-server/migrations/1719409568000-multiuser.js (1)
1-3: LGTM!The changes to import statements improve code consistency while maintaining the robust database schema design with proper foreign key constraints and reversible migrations.
packages/sync-server/src/app-gocardless/banks/hype_hyeeit22.js (1)
15-15: LGTM! Strict equality operators improve type safety.The changes from
==to===enhance type safety in transaction code handling, which is crucial for financial data processing.Verify edge case handling for payee names containing dashes.
As noted in the comments, there's a potential edge case where payee names containing dashes might cause issues with the remittance info parsing.
Also applies to: 28-29, 36-36, 47-47, 55-55
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Search for transactions with payee names containing dashes to verify the edge case rg -l "proprietaryBankTransactionCode.*[\"'](?:p2p|bon)[\"'].*remittanceInformationUnstructured.*-.*-" -g "!*.test.js"Length of output: 124
Action Required: Manual Verification for Dash Edge Case in Payee Names
The automated search did not reveal any evidence that payee names containing dashes are processed differently in the remittance info logic. While the strict equality adjustments improve type safety overall, the handling of the dash edge case remains inconclusive from the automated scan. Please manually verify that the code correctly processes remittance information where payee names include dashes, or consider adding targeted tests for this scenario.
packages/sync-server/jest.global-setup.js (1)
1-2: LGTM! Import statements updated to use named imports.The changes improve code maintainability by using named imports instead of default imports.
packages/sync-server/src/accounts/password.js (3)
2-2: LGTM! Import statements updated for better clarity.The changes improve code maintainability by:
- Using named imports instead of namespace imports for uuid
- Using consistent naming for the config import
Also applies to: 4-5
61-61: LGTM! Updated UUID generation to use named import.The change from
uuid.v4()touuidv4()is consistent with the import changes and improves code readability.Also applies to: 68-68
88-91: LGTM! Updated config references.The changes maintain consistency with the updated config import.
packages/sync-server/src/app-account.js (2)
17-17: LGTM! Import statement updated to use named imports consistently.The change improves code organization and maintainability.
69-69: LGTM! Updated to use strict equality.Changed from loose equality (
==) to strict equality (===) for better type safety.packages/sync-server/src/app-sync/services/files-service.js (3)
1-1: LGTM! Import statement updated to use named imports.The change improves code maintainability.
133-144: LGTM! Improved SQL query formatting.The SQL query formatting changes improve readability while maintaining functionality.
212-212: LGTM! Updated to use strict inequality.Changed from loose inequality (
!=) to strict inequality (!==) for better type safety.packages/sync-server/src/account-db.js (1)
7-8: LGTM! Import and export changes align with the PR objectives.The changes to use named imports and exports are consistent with the goal of re-enabling linting on the server.
Also applies to: 12-12
packages/sync-server/src/app-sync/tests/services/files-service.test.js (1)
3-3: LGTM! Import change aligns with the PR objectives.The change to use named import is consistent with the goal of re-enabling linting on the server.
packages/sync-server/src/accounts/openid.js (2)
2-2: LGTM! Import changes align with the PR objectives.The changes to use named imports are consistent with the goal of re-enabling linting on the server.
Also applies to: 4-5
210-210: LGTM! UUID generation updates are consistent.The changes to use the named
uuidv4function directly improve code clarity.Also applies to: 264-264
packages/sync-server/src/app-admin.js (2)
2-2: LGTM! Import changes align with the PR objectives.The changes to use named imports are consistent with the goal of re-enabling linting on the server.
Also applies to: 11-11
81-81: LGTM! UUID generation update is consistent.The change to use the named
uuidv4function directly improves code clarity.packages/sync-server/src/app-sync.js (1)
6-6: LGTM!The import statements have been updated to use named imports, which aligns with the linting rules and improves code consistency.
Also applies to: 8-8
packages/sync-server/src/app-simplefin/app-simplefin.js (1)
80-80: LGTM!The changes improve code quality by:
- Using strict equality (
!==) for array type comparisons- Using nullish coalescing (
??) for cleaner null/undefined checksAlso applies to: 208-208
packages/sync-server/src/app-gocardless/services/gocardless-service.js (1)
3-3: LGTM!The changes improve code consistency by:
- Using named imports instead of namespace imports
- Standardizing UUID generation using
uuidv4()Also applies to: 6-6, 322-322
packages/sync-server/src/app-sync.test.js (1)
7-7: LGTM! Import statement updated correctly.The change from default import to named import for
getAccountDbis consistent with the linting requirements.
Re-enables linting on the server. A couple of exceptions to get it through (I'm not sure on the bundler stuff, I can't work out how to remove the extensions from the imports) but can be fixed later.