sync-server eslint --fix #4359
sync-server eslint --fix #4359
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. |
WalkthroughThis pull request introduces extensive code clean-up and refactoring across multiple modules. The changes primarily consist of reorganizing import statements to improve dependency clarity and structure and updating variable declarations from 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: 0
🔭 Outside diff range comments (1)
packages/sync-server/src/app-account.js (1)
59-111: 🛠️ Refactor suggestionConsider implementing rate limiting for login attempts.
The login endpoint should implement rate limiting to prevent brute force attacks. Consider using a middleware like
express-rate-limit.+import rateLimit from 'express-rate-limit'; + +const loginLimiter = rateLimit({ + windowMs: 15 * 60 * 1000, // 15 minutes + max: 5, // Limit each IP to 5 login attempts per window + message: { status: 'error', reason: 'Too many login attempts' } +}); + -app.post('/login', async (req, res) => { +app.post('/login', loginLimiter, async (req, res) => {
🧹 Nitpick comments (4)
packages/sync-server/src/app-secrets.js (1)
20-23: Consider using more specific error handling for database operations.The database query could fail for various reasons (e.g., missing table, connection issues). Consider adding more specific error handling to help with debugging.
- const result = getAccountDb().first( - 'SELECT method FROM auth WHERE active = 1', - ); + const db = getAccountDb(); + if (!db) { + throw new Error('Failed to connect to database'); + } + const result = db.first('SELECT method FROM auth WHERE active = 1');packages/sync-server/src/app-gocardless/banks/fortuneo_ftnofrp1xxx.js (1)
31-44: Consider precompiling the regular expression for better performance.Since the regular expression is constant and used in a function that might be called frequently, consider precompiling it outside the function scope.
+const KEYWORDS_TO_REMOVE = [ + 'VIR INST', + 'VIR', + 'PRLV', + 'ANN CARTE', + 'CARTE \\d{2}\\/\\d{2}', +]; +const KEYWORDS_REGEX = new RegExp(KEYWORDS_TO_REMOVE.join('|'), 'g'); + /** @type {import('./bank.interface.js').IBank} */ export default { ...Fallback, institutionIds: ['FORTUNEO_FTNOFRP1XXX'], normalizeTransaction(transaction, _booked) { - const keywordsToRemove = [ - 'VIR INST', - 'VIR', - 'PRLV', - 'ANN CARTE', - 'CARTE \\d{2}\\/\\d{2}', - ]; const details = transaction.remittanceInformationUnstructuredArray.join(' '); const amount = transaction.transactionAmount.amount; - const regex = new RegExp(keywordsToRemove.join('|'), 'g'); - const payeeName = details.replace(regex, '').trim(); + const payeeName = details.replace(KEYWORDS_REGEX, '').trim();packages/sync-server/src/app-gocardless/banks/hype_hyeeit22.js (1)
47-68: Consider adding JSDoc comments for the unicode conversion logic.The unicode conversion logic is complex and would benefit from more detailed documentation explaining the algorithm and its constraints.
+/** + * Converts escaped unicode sequences in the format \Uxxxx to their corresponding characters. + * Assumes that unicode codepoints are always grouped in 4-byte bundles. + * @example + * Input: "Hello \\U0001\\U0002" + * Output: "Hello 🙂" (example emoji) + */ if (transaction.proprietaryBankTransactionCode == 'p2p') {packages/sync-server/src/accounts/password.js (1)
8-10: Consider strengthening password validation.The current password validation only checks for non-null and non-empty strings. Consider adding minimum length and complexity requirements.
function isValidPassword(password) { - return password != null && password !== ''; + const MIN_LENGTH = 8; + const hasUpperCase = /[A-Z]/.test(password); + const hasLowerCase = /[a-z]/.test(password); + const hasNumbers = /\d/.test(password); + return password != null && + password !== '' && + password.length >= MIN_LENGTH && + hasUpperCase && + hasLowerCase && + hasNumbers; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (71)
packages/sync-server/migrations/1694360000000-create-folders.js(1 hunks)packages/sync-server/migrations/1719409568000-multiuser.js(1 hunks)packages/sync-server/src/account-db.js(8 hunks)packages/sync-server/src/accounts/openid.js(9 hunks)packages/sync-server/src/accounts/password.js(6 hunks)packages/sync-server/src/app-account.js(8 hunks)packages/sync-server/src/app-admin.js(2 hunks)packages/sync-server/src/app-admin.test.js(1 hunks)packages/sync-server/src/app-gocardless/app-gocardless.js(3 hunks)packages/sync-server/src/app-gocardless/banks/abanca_caglesmm.js(1 hunks)packages/sync-server/src/app-gocardless/banks/abnamro_abnanl2a.js(1 hunks)packages/sync-server/src/app-gocardless/banks/american_express_aesudef1.js(1 hunks)packages/sync-server/src/app-gocardless/banks/bancsabadell_bsabesbbb.js(1 hunks)packages/sync-server/src/app-gocardless/banks/bank.interface.ts(1 hunks)packages/sync-server/src/app-gocardless/banks/bankinter_bkbkesmm.js(1 hunks)packages/sync-server/src/app-gocardless/banks/belfius_gkccbebb.js(1 hunks)packages/sync-server/src/app-gocardless/banks/berliner_sparkasse_beladebexxx.js(2 hunks)packages/sync-server/src/app-gocardless/banks/bnp_be_gebabebb.js(2 hunks)packages/sync-server/src/app-gocardless/banks/cbc_cregbebb.js(1 hunks)packages/sync-server/src/app-gocardless/banks/commerzbank_cobadeff.js(1 hunks)packages/sync-server/src/app-gocardless/banks/danskebank_dabno22.js(1 hunks)packages/sync-server/src/app-gocardless/banks/easybank_bawaatww.js(2 hunks)packages/sync-server/src/app-gocardless/banks/entercard_swednokk.js(1 hunks)packages/sync-server/src/app-gocardless/banks/fortuneo_ftnofrp1xxx.js(1 hunks)packages/sync-server/src/app-gocardless/banks/hype_hyeeit22.js(3 hunks)packages/sync-server/src/app-gocardless/banks/ing_ingddeff.js(1 hunks)packages/sync-server/src/app-gocardless/banks/ing_pl_ingbplpw.js(1 hunks)packages/sync-server/src/app-gocardless/banks/integration-bank.js(1 hunks)packages/sync-server/src/app-gocardless/banks/kbc_kredbebb.js(1 hunks)packages/sync-server/src/app-gocardless/banks/mbank_retail_brexplpw.js(1 hunks)packages/sync-server/src/app-gocardless/banks/nbg_ethngraaxxx.js(1 hunks)packages/sync-server/src/app-gocardless/banks/norwegian_xx_norwnok1.js(1 hunks)packages/sync-server/src/app-gocardless/banks/revolut_revolt21.js(1 hunks)packages/sync-server/src/app-gocardless/banks/sandboxfinance_sfin0000.js(1 hunks)packages/sync-server/src/app-gocardless/banks/seb_kort_bank_ab.js(1 hunks)packages/sync-server/src/app-gocardless/banks/seb_privat.js(1 hunks)packages/sync-server/src/app-gocardless/banks/sparnord_spnodk22.js(1 hunks)packages/sync-server/src/app-gocardless/banks/spk_karlsruhe_karsde66.js(2 hunks)packages/sync-server/src/app-gocardless/banks/spk_marburg_biedenkopf_heladef1mar.js(1 hunks)packages/sync-server/src/app-gocardless/banks/spk_worms_alzey_ried_malade51wor.js(1 hunks)packages/sync-server/src/app-gocardless/banks/ssk_dusseldorf_dussdeddxxx.js(1 hunks)packages/sync-server/src/app-gocardless/banks/tests/abanca_caglesmm.spec.js(1 hunks)packages/sync-server/src/app-gocardless/banks/tests/belfius_gkccbebb.spec.js(1 hunks)packages/sync-server/src/app-gocardless/banks/tests/easybank_bawaatww.spec.js(1 hunks)packages/sync-server/src/app-gocardless/banks/tests/ing_pl_ingbplpw.spec.js(1 hunks)packages/sync-server/src/app-gocardless/banks/tests/integration_bank.spec.js(1 hunks)packages/sync-server/src/app-gocardless/banks/tests/nationwide_naiagb21.spec.js(1 hunks)packages/sync-server/src/app-gocardless/banks/tests/ssk_dusseldorf_dussdeddxxx.spec.js(1 hunks)packages/sync-server/src/app-gocardless/banks/tests/virgin_nrnbgb22.spec.js(1 hunks)packages/sync-server/src/app-gocardless/services/gocardless-service.js(2 hunks)packages/sync-server/src/app-gocardless/services/tests/gocardless-service.spec.js(2 hunks)packages/sync-server/src/app-gocardless/tests/bank-factory.spec.js(1 hunks)packages/sync-server/src/app-openid.js(4 hunks)packages/sync-server/src/app-secrets.js(2 hunks)packages/sync-server/src/app-simplefin/app-simplefin.js(4 hunks)packages/sync-server/src/app-sync.js(12 hunks)packages/sync-server/src/app-sync.test.js(1 hunks)packages/sync-server/src/app-sync/tests/services/files-service.test.js(3 hunks)packages/sync-server/src/app-sync/validation.js(1 hunks)packages/sync-server/src/app.js(1 hunks)packages/sync-server/src/db.js(3 hunks)packages/sync-server/src/load-config.js(2 hunks)packages/sync-server/src/migrations.js(1 hunks)packages/sync-server/src/scripts/health-check.js(1 hunks)packages/sync-server/src/secrets.test.js(1 hunks)packages/sync-server/src/services/secrets-service.js(1 hunks)packages/sync-server/src/sync-simple.js(4 hunks)packages/sync-server/src/util/middlewares.js(2 hunks)packages/sync-server/src/util/paths.js(1 hunks)packages/sync-server/src/util/prompt.js(3 hunks)packages/sync-server/src/util/validate-user.js(3 hunks)
✅ Files skipped from review due to trivial changes (46)
- packages/sync-server/src/app-gocardless/banks/commerzbank_cobadeff.js
- packages/sync-server/src/app-gocardless/banks/ing_ingddeff.js
- packages/sync-server/src/app-gocardless/banks/tests/abanca_caglesmm.spec.js
- packages/sync-server/src/migrations.js
- packages/sync-server/src/app-gocardless/banks/nbg_ethngraaxxx.js
- packages/sync-server/src/app-admin.test.js
- packages/sync-server/src/app-gocardless/banks/tests/ing_pl_ingbplpw.spec.js
- packages/sync-server/src/app-gocardless/banks/abanca_caglesmm.js
- packages/sync-server/src/app-gocardless/banks/spk_marburg_biedenkopf_heladef1mar.js
- packages/sync-server/src/app-gocardless/banks/mbank_retail_brexplpw.js
- packages/sync-server/src/app-gocardless/banks/sandboxfinance_sfin0000.js
- packages/sync-server/src/util/middlewares.js
- packages/sync-server/src/app-gocardless/banks/norwegian_xx_norwnok1.js
- packages/sync-server/src/app-gocardless/banks/abnamro_abnanl2a.js
- packages/sync-server/src/app-gocardless/banks/bancsabadell_bsabesbbb.js
- packages/sync-server/src/app-gocardless/banks/sparnord_spnodk22.js
- packages/sync-server/src/app-gocardless/banks/entercard_swednokk.js
- packages/sync-server/src/app-gocardless/banks/tests/ssk_dusseldorf_dussdeddxxx.spec.js
- packages/sync-server/src/app-sync.test.js
- packages/sync-server/src/app-gocardless/banks/american_express_aesudef1.js
- packages/sync-server/src/app-gocardless/banks/tests/belfius_gkccbebb.spec.js
- packages/sync-server/src/app-gocardless/banks/spk_worms_alzey_ried_malade51wor.js
- packages/sync-server/src/app-gocardless/banks/danskebank_dabno22.js
- packages/sync-server/src/app.js
- packages/sync-server/src/secrets.test.js
- packages/sync-server/src/app-gocardless/banks/bank.interface.ts
- packages/sync-server/src/app-gocardless/banks/ing_pl_ingbplpw.js
- packages/sync-server/src/app-gocardless/banks/seb_kort_bank_ab.js
- packages/sync-server/src/app-sync/tests/services/files-service.test.js
- packages/sync-server/src/app-gocardless/banks/belfius_gkccbebb.js
- packages/sync-server/src/app-gocardless/banks/tests/easybank_bawaatww.spec.js
- packages/sync-server/src/app-gocardless/banks/kbc_kredbebb.js
- packages/sync-server/migrations/1719409568000-multiuser.js
- packages/sync-server/src/app-gocardless/services/gocardless-service.js
- packages/sync-server/src/db.js
- packages/sync-server/src/app-gocardless/banks/bankinter_bkbkesmm.js
- packages/sync-server/src/load-config.js
- packages/sync-server/src/scripts/health-check.js
- packages/sync-server/src/app-gocardless/tests/bank-factory.spec.js
- packages/sync-server/src/app-gocardless/services/tests/gocardless-service.spec.js
- packages/sync-server/src/app-gocardless/banks/cbc_cregbebb.js
- packages/sync-server/src/app-gocardless/banks/tests/integration_bank.spec.js
- packages/sync-server/src/app-gocardless/banks/seb_privat.js
- packages/sync-server/src/app-admin.js
- packages/sync-server/src/app-sync/validation.js
- packages/sync-server/src/app-simplefin/app-simplefin.js
🧰 Additional context used
🧠 Learnings (4)
packages/sync-server/src/app-gocardless/banks/tests/nationwide_naiagb21.spec.js (1)
Learnt from: matt-fidd
PR: actualbudget/actual#4253
File: packages/sync-server/src/app-gocardless/banks/nationwide_naiagb21.js:41-44
Timestamp: 2025-02-11T17:25:39.615Z
Learning: In bank sync providers, core fields like transactionId and amount should be mutated directly on the transaction object as they aren't covered by the fallback normalization logic and shouldn't be exposed for user mapping. Display-related fields should use editedTrans.
packages/sync-server/src/app-gocardless/banks/ssk_dusseldorf_dussdeddxxx.js (1)
Learnt from: matt-fidd
PR: actualbudget/actual#4253
File: packages/sync-server/src/app-gocardless/banks/integration-bank.js:68-71
Timestamp: 2025-02-11T17:28:02.196Z
Learning: In the integration-bank.js normalizeTransaction method, mutating the original transaction object by adding remittanceInformationUnstructuredArrayString and remittanceInformationStructuredArrayString properties is acceptable.
packages/sync-server/src/app-gocardless/banks/easybank_bawaatww.js (1)
Learnt from: matt-fidd
PR: actualbudget/actual#4253
File: packages/sync-server/src/app-gocardless/banks/nationwide_naiagb21.js:41-44
Timestamp: 2025-02-11T17:25:39.615Z
Learning: In bank sync providers, core fields like transactionId and amount should be mutated directly on the transaction object as they aren't covered by the fallback normalization logic and shouldn't be exposed for user mapping. Display-related fields should use editedTrans.
packages/sync-server/src/app-gocardless/banks/tests/virgin_nrnbgb22.spec.js (1)
Learnt from: matt-fidd
PR: actualbudget/actual#4253
File: packages/sync-server/src/app-gocardless/banks/virgin_nrnbgb22.js:25-30
Timestamp: 2025-02-11T17:26:19.364Z
Learning: The regex match validation in virgin_nrnbgb22.js was intentionally left as-is during the code copy to maintain consistency with the source code.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Functional
- GitHub Check: Visual regression
- GitHub Check: Build Docker image (alpine)
🔇 Additional comments (31)
packages/sync-server/src/app-gocardless/banks/spk_karlsruhe_karsde66.js (2)
2-4: LGTM!The reordering of imports improves code organization while maintaining functionality.
47-50: LGTM!Adding braces to the if block improves code clarity and consistency.
packages/sync-server/src/app-gocardless/banks/easybank_bawaatww.js (2)
1-1: LGTM! Import reordering looks good.The reordering of imports improves code organization while maintaining functionality.
Also applies to: 6-7
39-39: LGTM! Object property shorthand looks good.The use of object property shorthand improves code conciseness while maintaining functionality.
packages/sync-server/src/app-gocardless/banks/berliner_sparkasse_beladebexxx.js (2)
2-4: LGTM!The import reordering aligns with the PR's linting objectives and has no functional impact.
47-50: LGTM!The addition of braces improves code clarity while maintaining the same functionality.
packages/sync-server/src/app-gocardless/banks/ssk_dusseldorf_dussdeddxxx.js (1)
28-35: LGTM! Clean and explicit handling of additional information.The block structure improves readability and properly handles the combination of remittance information. The use of
filter(Boolean)elegantly handles potential falsy values.packages/sync-server/src/app-gocardless/banks/tests/virgin_nrnbgb22.spec.js (1)
2-2: LGTM!The import statement change aligns with the PR's objective of applying linting fixes.
packages/sync-server/src/app-gocardless/banks/tests/nationwide_naiagb21.spec.js (1)
2-2: LGTM!The import statement change aligns with the PR's objective of applying linting fixes.
packages/sync-server/src/util/validate-user.js (3)
2-4: LGTM! Import reorganization looks good.The import statement reordering aligns with standard linting practices while maintaining functionality.
20-20: LGTM! Appropriate use of const declaration.Converting to
constis correct as thesessionvariable is not reassigned after initialization.
51-52: LGTM! Appropriate use of const declarations.Converting
peer,peerIp, andmatchedtoconstis correct as these variables are not reassigned after initialization.Also applies to: 58-58
packages/sync-server/src/app-gocardless/banks/revolut_revolt21.js (1)
19-35: Verify the inconsistent usage offormatPayeeName.The
formatPayeeNamefunction is used inconsistently in the two conditional branches:
- For "Bizum payment from", a manual string replacement is used.
- For "Bizum payment to", the
formatPayeeNamefunction is used.This inconsistency might lead to different formatting of payee names based on the payment direction.
Please confirm if this inconsistency is intentional or if we should standardize the payee name formatting across both cases.
Also applies to: 43-56
packages/sync-server/src/app-gocardless/banks/integration-bank.js (1)
2-3: LGTM!The import statement is correctly placed and the
formatPayeeNamefunction is properly used in the base implementation, providing consistent payee name formatting for all bank integrations that use this as a fallback.packages/sync-server/src/util/paths.js (1)
1-13: LGTM!The code is well-structured with proper import organization, JSDoc documentation, and safe path construction using
join.packages/sync-server/migrations/1694360000000-create-folders.js (1)
1-26: LGTM!The migration is well-structured with proper error handling and cleanup. The use of
fs.promisesAPI and appropriate error codes (EEXIST) shows good practices.packages/sync-server/src/app-secrets.js (1)
35-35: LGTM!Good change from
lettoconstascanSaveSecretsis not reassigned.packages/sync-server/src/app-gocardless/banks/fortuneo_ftnofrp1xxx.js (1)
1-5: LGTM!The import organization is clean and logical.
packages/sync-server/src/util/prompt.js (1)
4-4: LGTM! Appropriate conversion of variables toconstThe changes correctly convert variables to
constwhere they are not reassigned after initialization, improving code immutability.Also applies to: 9-9, 16-16, 22-22, 29-29
packages/sync-server/src/services/secrets-service.js (1)
2-3: LGTM! Clean import organizationThe addition of
getAccountDbimport and the newline improves code organization by properly grouping imports.packages/sync-server/src/sync-simple.js (2)
11-12: LGTM! Appropriate conversion of variables toconstThe changes correctly convert variables to
constwhere they are not reassigned after initialization, improving code immutability.Also applies to: 30-31, 61-61, 73-74, 81-81
25-25: Verify remainingletdeclarationsThe following variables remain as
let, which appears correct as they are reassigned:
returnValue(line 25)trie(line 27)Also applies to: 27-27, 54-54
packages/sync-server/src/app-openid.js (1)
9-13: LGTM! Clean import organization and appropriate variable declarationsThe changes improve code quality through:
- Better import organization with logical grouping
- Correct conversion of variables to
constwhere they are not reassignedAlso applies to: 15-15, 31-31, 50-50, 87-87
packages/sync-server/src/app-gocardless/banks/hype_hyeeit22.js (1)
33-34: LGTM! Variable declarations correctly updated to useconst.The changes from
lettoconstforinfoIdxandnext_idxare appropriate as these variables are not reassigned after initialization.Also applies to: 54-54
packages/sync-server/src/app-gocardless/banks/bnp_be_gebabebb.js (1)
31-39: LGTM! Variable declarations correctly updated to useconst.The changes from
lettoconstforadditionalInformationObject,matches, and loop variablematchare appropriate as these variables are not reassigned after initialization.packages/sync-server/src/accounts/password.js (1)
3-7: LGTM! Import statements are well organized.The imports are properly grouped and ordered.
packages/sync-server/src/app-account.js (1)
2-18: LGTM! Import statements are well organized.The imports are properly grouped and ordered.
packages/sync-server/src/account-db.js (1)
2-10: LGTM! Import organization and variable declarations look good.The changes improve code clarity by:
- Grouping related imports together
- Using
constfor variables that are not reassignedpackages/sync-server/src/app-gocardless/app-gocardless.js (1)
4-20: LGTM! Import organization follows best practices.The changes improve code structure by:
- Organizing imports in a logical order (built-ins → third-party → local)
- Using
constappropriately for non-reassigned variablespackages/sync-server/src/accounts/openid.js (1)
2-10: LGTM! Import organization and variable declarations are appropriate.The changes enhance code quality by:
- Organizing imports logically
- Converting variables to
constwhere they're not reassigned- Retaining
letwhere variable reassignment is neededpackages/sync-server/src/app-sync.js (1)
2-26: LGTM! Import organization and variable declarations are well structured.The changes improve code quality by:
- Organizing imports logically (node built-ins → third-party → local)
- Using
constfor non-reassigned variables- Retaining
letwhere reassignment is needed
|
Closed in favour of #4362 |
This is just the lint autofixes to start bringing the sync server in line with the linting rules in the main repo
Linting is still disabled while the rest of the issues are looked at.
The CodeQL errors aren't introduced here, but the lines around them have been edited.
I didn't include a release note here, there's no meaningful change but I can if you'd like.