Fix error spread in non-batch bank sync #4689
Conversation
actual-github-bot
bot
changed the title
mariolamacchia
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
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
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
Smaller No assets were smaller Unchanged No assets were unchanged |
WalkthroughThis pull request introduces a new test file Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
🪧 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: 3
🧹 Nitpick comments (2)
packages/loot-core/src/server/api.test.ts (2)
1-6: Reorder imports to match project conventionsAccording to the linter, the imports need to be reordered and grouped properly.
-import { installAPI } from './api'; -import { getBankSyncError } from '../shared/errors'; +import { getBankSyncError } from '../shared/errors'; + +import { installAPI } from './api';🧰 Tools
🪛 GitHub Check: lint
[warning] 2-2:
../shared/errorsimport should occur before import of./api
[warning] 1-1:
There should be at least one empty line between import groups
21-23: Avoid using the delete operator for performance reasonsThe static analysis tool warns against using the
deleteoperator due to potential performance impacts.- delete handlers['accounts-bank-sync']; + handlers['accounts-bank-sync'] = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 22-22: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🪛 GitHub Check: typecheck
[failure] 21-21:
Variable 'handlers' implicitly has an 'any' type.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4689.mdis excluded by!**/*.md
📒 Files selected for processing (2)
packages/loot-core/src/server/api.test.ts(1 hunks)packages/loot-core/src/server/api.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
packages/loot-core/src/server/api.test.ts (1)
packages/loot-core/src/server/api.ts (1)
installAPI(769-773)
🪛 Biome (1.9.4)
packages/loot-core/src/server/api.test.ts
[error] 22-22: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🪛 GitHub Check: lint
packages/loot-core/src/server/api.test.ts
[warning] 2-2:
../shared/errors import should occur before import of ./api
[warning] 1-1:
There should be at least one empty line between import groups
🪛 GitHub Check: typecheck
packages/loot-core/src/server/api.test.ts
[failure] 42-42:
Variable 'handlers' implicitly has an 'any' type.
[failure] 37-37:
Variable 'mockServerHandlers' implicitly has an 'any' type.
[failure] 31-31:
Variable 'mockServerHandlers' implicitly has an 'any' type.
[failure] 30-30:
Variable 'handlers' implicitly has an 'any' type.
[failure] 21-21:
Variable 'handlers' implicitly has an 'any' type.
[failure] 16-16:
Object literal may only specify known properties, and ''accounts-bank-sync'' does not exist in type 'ServerHandlers'.
[failure] 10-10:
Variable 'mockServerHandlers' implicitly has type 'any' in some locations where its type cannot be determined.
[failure] 9-9:
Variable 'handlers' implicitly has type 'any' in some locations where its type cannot be determined.
🪛 GitHub Actions: Test
packages/loot-core/src/server/api.test.ts
[error] 9-9: error TS7034: Variable 'handlers' implicitly has type 'any' in some locations where its type cannot be determined.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Functional
- GitHub Check: Visual regression
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (windows-latest)
🔇 Additional comments (3)
packages/loot-core/src/server/api.ts (1)
261-261: Fixed error collection in non-batch bank syncThe change from
allErrors.push(errors)toallErrors.push(...errors)correctly flattens the errors array rather than nesting it. This ensures that errors from non-batch syncs are properly aggregated and can be detected in the subsequent error filtering and handling.packages/loot-core/src/server/api.test.ts (2)
28-35: Test for single account sync - LGTMThis is a well-structured test that verifies the
api/bank-synchandler correctly passes the account ID to the underlyingaccounts-bank-synchandler when in non-batch mode.🧰 Tools
🪛 GitHub Check: typecheck
[failure] 31-31:
Variable 'mockServerHandlers' implicitly has an 'any' type.
[failure] 30-30:
Variable 'handlers' implicitly has an 'any' type.
36-47: Test for error handling in non-batch sync - LGTMGood test that verifies error handling in non-batch sync mode. It ensures that errors from the
accounts-bank-synchandler are properly thrown and formatted usinggetBankSyncError.🧰 Tools
🪛 GitHub Check: typecheck
[failure] 42-42:
Variable 'handlers' implicitly has an 'any' type.
[failure] 37-37:
Variable 'mockServerHandlers' implicitly has an 'any' type.
| describe('API handlers', () => { | ||
| let handlers; | ||
| let mockServerHandlers; | ||
|
|
||
| beforeEach(() => { | ||
| jest.clearAllMocks(); | ||
|
|
||
| mockServerHandlers = { | ||
| 'accounts-bank-sync': jest.fn().mockResolvedValue({ errors: [] }), | ||
| }; | ||
|
|
||
| // Remove the accounts-bank-sync handler if it exists | ||
| // or it won't be replaced by the mock | ||
| if (handlers) { | ||
| delete handlers['accounts-bank-sync']; | ||
| } | ||
|
|
||
| handlers = installAPI(mockServerHandlers); | ||
| }); | ||
|
|
||
| describe('api/bank-sync', () => { | ||
| it('should sync a single account when accountId is provided', async () => { | ||
| await handlers['api/bank-sync']({ accountId: 'account1' }); | ||
| expect(mockServerHandlers['accounts-bank-sync']).toHaveBeenCalledWith({ | ||
| ids: ['account1'], | ||
| }); | ||
| }); | ||
|
|
||
| it('should handle errors in non batch sync', async () => { | ||
| mockServerHandlers['accounts-bank-sync'].mockResolvedValue({ | ||
| errors: ['connection-failed'], | ||
| }); | ||
|
|
||
| await expect( | ||
| handlers['api/bank-sync']({ accountId: 'account2' }), | ||
| ).rejects.toThrow('Bank sync error: connection-failed'); | ||
|
|
||
| expect(getBankSyncError).toHaveBeenCalledWith('connection-failed'); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add TypeScript type imports to test file
To address the TypeScript errors throughout the file, add the necessary type imports from server-handlers.
import { installAPI } from './api';
import { getBankSyncError } from '../shared/errors';
+import { Handlers } from '../types/handlers';
+import { ServerHandlers } from '../types/server-handlers';
jest.mock('../shared/errors', () => ({
getBankSyncError: jest.fn(error => `Bank sync error: ${error}`),
}));
describe('API handlers', () => {
- let handlers;
- let mockServerHandlers;
+ let handlers: Handlers;
+ let mockServerHandlers: Partial<ServerHandlers>;This will properly address the TypeScript errors throughout the file and make the code more maintainable.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| describe('API handlers', () => { | |
| let handlers; | |
| let mockServerHandlers; | |
| beforeEach(() => { | |
| jest.clearAllMocks(); | |
| mockServerHandlers = { | |
| 'accounts-bank-sync': jest.fn().mockResolvedValue({ errors: [] }), | |
| }; | |
| // Remove the accounts-bank-sync handler if it exists | |
| // or it won't be replaced by the mock | |
| if (handlers) { | |
| delete handlers['accounts-bank-sync']; | |
| } | |
| handlers = installAPI(mockServerHandlers); | |
| }); | |
| describe('api/bank-sync', () => { | |
| it('should sync a single account when accountId is provided', async () => { | |
| await handlers['api/bank-sync']({ accountId: 'account1' }); | |
| expect(mockServerHandlers['accounts-bank-sync']).toHaveBeenCalledWith({ | |
| ids: ['account1'], | |
| }); | |
| }); | |
| it('should handle errors in non batch sync', async () => { | |
| mockServerHandlers['accounts-bank-sync'].mockResolvedValue({ | |
| errors: ['connection-failed'], | |
| }); | |
| await expect( | |
| handlers['api/bank-sync']({ accountId: 'account2' }), | |
| ).rejects.toThrow('Bank sync error: connection-failed'); | |
| expect(getBankSyncError).toHaveBeenCalledWith('connection-failed'); | |
| }); | |
| }); | |
| }); | |
| import { installAPI } from './api'; | |
| import { getBankSyncError } from '../shared/errors'; | |
| import { Handlers } from '../types/handlers'; | |
| import { ServerHandlers } from '../types/server-handlers'; | |
| jest.mock('../shared/errors', () => ({ | |
| getBankSyncError: jest.fn(error => `Bank sync error: ${error}`), | |
| })); | |
| describe('API handlers', () => { | |
| let handlers: Handlers; | |
| let mockServerHandlers: Partial<ServerHandlers>; | |
| beforeEach(() => { | |
| jest.clearAllMocks(); | |
| mockServerHandlers = { | |
| 'accounts-bank-sync': jest.fn().mockResolvedValue({ errors: [] }), | |
| }; | |
| // Remove the accounts-bank-sync handler if it exists | |
| // or it won't be replaced by the mock | |
| if (handlers) { | |
| delete handlers['accounts-bank-sync']; | |
| } | |
| handlers = installAPI(mockServerHandlers); | |
| }); | |
| describe('api/bank-sync', () => { | |
| it('should sync a single account when accountId is provided', async () => { | |
| await handlers['api/bank-sync']({ accountId: 'account1' }); | |
| expect(mockServerHandlers['accounts-bank-sync']).toHaveBeenCalledWith({ | |
| ids: ['account1'], | |
| }); | |
| }); | |
| it('should handle errors in non batch sync', async () => { | |
| mockServerHandlers['accounts-bank-sync'].mockResolvedValue({ | |
| errors: ['connection-failed'], | |
| }); | |
| await expect( | |
| handlers['api/bank-sync']({ accountId: 'account2' }), | |
| ).rejects.toThrow('Bank sync error: connection-failed'); | |
| expect(getBankSyncError).toHaveBeenCalledWith('connection-failed'); | |
| }); | |
| }); | |
| }); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 22-22: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🪛 GitHub Check: typecheck
[failure] 42-42:
Variable 'handlers' implicitly has an 'any' type.
[failure] 37-37:
Variable 'mockServerHandlers' implicitly has an 'any' type.
[failure] 31-31:
Variable 'mockServerHandlers' implicitly has an 'any' type.
[failure] 30-30:
Variable 'handlers' implicitly has an 'any' type.
[failure] 21-21:
Variable 'handlers' implicitly has an 'any' type.
[failure] 16-16:
Object literal may only specify known properties, and ''accounts-bank-sync'' does not exist in type 'ServerHandlers'.
[failure] 10-10:
Variable 'mockServerHandlers' implicitly has type 'any' in some locations where its type cannot be determined.
[failure] 9-9:
Variable 'handlers' implicitly has type 'any' in some locations where its type cannot be determined.
🪛 GitHub Actions: Test
[error] 9-9: error TS7034: Variable 'handlers' implicitly has type 'any' in some locations where its type cannot be determined.
|
Hey! This would be nice to get in before the release in a couple of days if possible, could you take a look at the failing CI when you get a chance? |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…udget#4683) fix issue where filters set in the widget editor for the cash flow widget were not applied in the Reports dashboard
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/loot-core/src/server/api.test.ts (2)
1-10:⚠️ Potential issueFix TypeScript type errors by importing proper types.
The current type definitions are causing pipeline failures because
Record<string, jest.Mock>is not assignable to theServerHandlerstype expected byinstallAPI. Fix this by importing the correct types.+import { Handlers } from '../types/handlers'; +import { ServerHandlers } from '../types/server-handlers'; import { installAPI } from './api'; import { getBankSyncError } from '../shared/errors'; jest.mock('../shared/errors', () => ({ getBankSyncError: jest.fn(error => `Bank sync error: ${error}`), })); describe('API handlers', () => { - let handlers: Record<string, any>; - let mockServerHandlers: Record<string, jest.Mock>; + let handlers: Handlers; + let mockServerHandlers: Partial<ServerHandlers>;🧰 Tools
🪛 ESLint
[error] 9-9: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
🪛 GitHub Check: lint
[failure] 9-9:
Unexpected any. Specify a different type
[warning] 2-2:
../shared/errorsimport should occur before import of./api
[warning] 1-1:
There should be at least one empty line between import groups
15-17:⚠️ Potential issueFix type assertion for mockServerHandlers.
The current object literal doesn't match the
ServerHandlerstype expected byinstallAPI. Correct it with a proper type assertion.mockServerHandlers = { 'accounts-bank-sync': jest.fn().mockResolvedValue({ errors: [] }), -}; +} as Partial<ServerHandlers>;
🧹 Nitpick comments (2)
packages/loot-core/src/server/api.test.ts (2)
1-3: Fix import ordering to follow project's conventions.The imports should be organized alphabetically with external imports coming before local imports.
+import { getBankSyncError } from '../shared/errors'; import { installAPI } from './api'; -import { getBankSyncError } from '../shared/errors';🧰 Tools
🪛 GitHub Check: lint
[warning] 2-2:
../shared/errorsimport should occur before import of./api
[warning] 1-1:
There should be at least one empty line between import groups
21-23: Replace delete operator with undefined assignment for better performance.Using the delete operator can impact performance. Use undefined assignment instead.
if (handlers) { - delete handlers['accounts-bank-sync']; + handlers['accounts-bank-sync'] = undefined; }🧰 Tools
🪛 Biome (1.9.4)
[error] 22-22: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/loot-core/src/server/api.test.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
packages/loot-core/src/server/api.test.ts (1)
packages/loot-core/src/server/api.ts (1)
installAPI(769-773)
🪛 Biome (1.9.4)
packages/loot-core/src/server/api.test.ts
[error] 22-22: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🪛 ESLint
packages/loot-core/src/server/api.test.ts
[error] 9-9: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
🪛 GitHub Check: typecheck
packages/loot-core/src/server/api.test.ts
[failure] 25-25:
Argument of type 'Record<string, Mock<any, any, any>>' is not assignable to parameter of type 'ServerHandlers'.
🪛 GitHub Check: lint
packages/loot-core/src/server/api.test.ts
[failure] 9-9:
Unexpected any. Specify a different type
[warning] 2-2:
../shared/errors import should occur before import of ./api
[warning] 1-1:
There should be at least one empty line between import groups
🪛 GitHub Actions: Test
packages/loot-core/src/server/api.test.ts
[error] 25-27: error TS2345: Argument of type 'Record<string, Mock<any, any, any>>' is not assignable to parameter of type 'ServerHandlers'.
🪛 GitHub Actions: Build
packages/loot-core/src/server/api.test.ts
[error] 25-27: Argument of type 'Record<string, Mock<any, any, any>>' is not assignable to parameter of type 'ServerHandlers'.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Visual regression
- GitHub Check: Functional
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (windows-latest)
🔇 Additional comments (2)
packages/loot-core/src/server/api.test.ts (2)
29-34: LGTM: Test correctly validates single account sync behavior.The test properly verifies that when an accountId is provided, the 'accounts-bank-sync' handler is called with the correct parameters.
36-46: LGTM: Test properly validates error handling in non-batch sync.This test correctly verifies that:
- Errors from 'accounts-bank-sync' are properly thrown
- The getBankSyncError function is called with the correct error code
This directly addresses the PR's objective of fixing error handling in non-batch bank sync.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/loot-core/src/server/api.test.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: lint
packages/loot-core/src/server/api.test.ts
[warning] 1-1:
./api import should occur after import of ../types/server-handlers
[warning] 1-1:
There should be at least one empty line between import groups
🪛 GitHub Actions: Test
packages/loot-core/src/server/api.test.ts
[warning] 1-1: There should be at least one empty line between import groups (import/order)
[warning] 1-1: ./api import should occur after import of ../types/server-handlers (import/order)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Functional
- GitHub Check: Visual regression
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (3)
packages/loot-core/src/server/api.test.ts (3)
9-11: LGTM - Good job adding proper type annotations.The type assertion for ServerHandlers properly resolves the TypeScript errors mentioned in previous reviews.
12-22: Good test for single account sync functionality.This test correctly verifies that when an accountId is provided, the 'accounts-bank-sync' handler is called with the appropriate parameters.
24-35: Well-implemented error handling test.This test effectively validates the key issue from the PR objectives - ensuring that errors are properly captured during non-batch bank sync operations. The test verifies both the thrown error and that the error formatting function is called correctly.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@matt-fidd fixed! Thanks for taking a look |
Fixed an issue in the bank sync API where errors weren't being properly collected during bank sync, which led to non-batch syncs to always fail.