[WIP] Use JS Proxy to simplify loot-core app calls from desktop-client#7246
Open
joel-jeremy wants to merge 10 commits intomasterfrom
Open
[WIP] Use JS Proxy to simplify loot-core app calls from desktop-client#7246joel-jeremy wants to merge 10 commits intomasterfrom
joel-jeremy wants to merge 10 commits intomasterfrom
Conversation
… connection package's `send`
…nside app and update all calls to go through the mainApp for consistency.
…aturally i.e. app.createPayee(...) vs. app.runHandler('createPayee', ...)
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
joel-jeremy
changed the title
joel-jeremy
changed the title
This comment has been minimized.
This comment has been minimized.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces ad-hoc string-based Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (UI)
participant Proxy as ServerProxy
participant Conn as Connection
participant ServerApp as mainApp
participant Handler as Handler
rect rgba(120,180,240,0.5)
Note over Client,Handler: Client → typed proxy → connection → server app → handler
Client->>Proxy: server.createPayee({ name })
Proxy->>Conn: send('createPayee', { name })
Conn->>ServerApp: deliver message
ServerApp->>Handler: runHandler('createPayee', args)
Handler-->>ServerApp: result
ServerApp-->>Conn: reply(result)
Conn-->>Proxy: resolve promise
Proxy-->>Client: result
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/loot-core/src/server/importers/ynab5.ts (1)
451-461:⚠️ Potential issue | 🟠 MajorCreate imported payees sequentially.
This still fans out
api/payee-createthroughPromise.all(). The payee creation path has already hit duplicate-payee races under concurrent processing, so importing payees in parallel can recreate that bug when names collide after normalization.Based on learnings: "When finalizing transactions that involve inserting or retrieving payees, avoid using `Promise.all` as it may result in duplicate payees due to concurrent operations. Sequential processing ensures payees are correctly handled without duplication."🐛 Proposed fix
-function importPayees(data: Budget, entityIdMap: Map<string, string>) { - return Promise.all( - data.payees.map(async payee => { - if (!payee.deleted) { - const id = await mainApp['api/payee-create']({ - payee: { name: payee.name }, - }); - entityIdMap.set(payee.id, id); - } - }), - ); +async function importPayees(data: Budget, entityIdMap: Map<string, string>) { + for (const payee of data.payees) { + if (payee.deleted) { + continue; + } + + const id = await mainApp['api/payee-create']({ + payee: { name: payee.name }, + }); + entityIdMap.set(payee.id, id); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/loot-core/src/server/importers/ynab5.ts` around lines 451 - 461, importPayees currently fans out api/payee-create with Promise.all causing duplicate-payee races; change it to process payees sequentially by iterating (e.g., for...of) over data.payees, skipping deleted, awaiting each mainApp['api/payee-create'] call before proceeding, and setting entityIdMap.set(payee.id, id) immediately after each awaited call to ensure serial creation and avoid concurrent name-normalization collisions.
🧹 Nitpick comments (3)
packages/loot-core/src/platform/client/connection/index-types.ts (1)
7-15: Keep the proxy signature aligned with the transport contract.
args?makes everyserver.*method callable with no payload, so the new proxy still lets calls likeserver.createPayee()type-check. This surface is supposed to be the typed replacement forsend(...), so I'd preserve the handler parameter list exactly and make the proxy explicitly promise-returning.♻️ Proposed type shape
export type ServerProxy = { - [K in keyof Handlers]: ( - args?: Parameters<Handlers[K]>[0], - ) => ReturnType<Handlers[K]>; + [K in keyof Handlers]: ( + ...args: Parameters<Handlers[K]> + ) => Promise<Awaited<ReturnType<Handlers[K]>>>; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/loot-core/src/platform/client/connection/index-types.ts` around lines 7 - 15, The proxy type ServerProxy currently makes the handler argument optional and doesn't force a Promise result; update ServerProxy so each method preserves the handler parameter list exactly (use Parameters<Handlers[K]> to keep the same parameter tuple instead of a single optional args) and make the return type explicitly promise-returning (wrap/await the handler's ReturnType so server.* methods return Promise of the handler result). Apply the same corrected signature to the declared server and the init() return type so server and init() align with the transport contract.packages/loot-core/src/mocks/budget.ts (2)
746-747: Usingconsole.errorin loot-core.As per coding guidelines,
loot-coreshould use logger instead of console. However, this is mock/test code, so this may be acceptable.Based on learnings: "Custom ESLint rule
actual/prefer-logger-over-consoleenforces using logger instead of console inpackages/loot-core/"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/loot-core/src/mocks/budget.ts` around lines 746 - 747, Replace the direct console.error call with the project's logger (e.g., logger.error or getLogger().error) so the mock uses the centralized logging API; specifically, in the block that currently does console.error('Unknown account name for test budget: ', account.name) (near the fillChecking(app, account, payees, allGroups) call), import or obtain the logger instance at the top of the file and call logger.error with the same message and account.name (or interpolate) to satisfy the actual/prefer-logger-over-console rule while keeping behavior unchanged.
300-303: Consider using direct handler calls instead ofrunHandlerfor consistency.The helper functions use
app.runHandler('transactions-batch-update', ...)while other places useapp['handler-name'](). Iftransactions-batch-updatehandler exists on the type, you could useapp['transactions-batch-update']()for consistency.However, if this is intentional for the incremental migration (keeping kebab-case handlers working via
runHandler), this is acceptable.Also applies to: 336-339, 381-384, 418-421, 456-459
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/loot-core/src/mocks/budget.ts` around lines 300 - 303, Replace uses of app.runHandler('transactions-batch-update', {...}) with the direct handler call style app['transactions-batch-update']({...}) for consistency with other helpers; locate occurrences of app.runHandler('transactions-batch-update', ...) (including the similar calls later in the file) and change them to the bracketed handler invocation (app['transactions-batch-update'](...)) only if that handler exists on the app type, otherwise leave runHandler if this kebab-case indirection is intentionally required for incremental migration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/loot-core/src/platform/client/connection/index.ts`:
- Around line 102-107: init() currently caches the connection promise in
initPromise but clearServer() doesn't reset it, so calling clearServer() leaves
initPromise pointing to a stale/closed socket; update clearServer() to set
initPromise = undefined (or null) after closing/clearing the socket and any
related state so that the next call to init() will call new
Promise(connectSocket) again, referencing the symbols init, clearServer,
initPromise, and connectSocket to locate and modify the code.
In `@packages/loot-core/src/server/api.ts`:
- Line 86: handlers is initialized empty but api handler closures call delegated
methods like 'load-budget', 'get-budgets', 'close-budget' that are never
assigned, causing runtime undefined calls; fix by wiring the non-API delegate
methods into the local handlers object before calling createApp()—for example,
import or obtain the BudgetFileHandlers (the object that implements
'load-budget', 'get-budgets', 'download-budget', etc.) and merge its methods
into the local handlers (e.g., Object.assign(handlers, budgetFileHandlers)) or
otherwise assign each delegate function to handlers['load-budget'],
handlers['get-budgets'], handlers['close-budget'], etc., ensuring types still
satisfy ApiHandlers, or alternatively refactor the api handler closures to call
the combined app/mainApp that already registers those delegate methods instead
of referencing the local handlers bag.
---
Outside diff comments:
In `@packages/loot-core/src/server/importers/ynab5.ts`:
- Around line 451-461: importPayees currently fans out api/payee-create with
Promise.all causing duplicate-payee races; change it to process payees
sequentially by iterating (e.g., for...of) over data.payees, skipping deleted,
awaiting each mainApp['api/payee-create'] call before proceeding, and setting
entityIdMap.set(payee.id, id) immediately after each awaited call to ensure
serial creation and avoid concurrent name-normalization collisions.
---
Nitpick comments:
In `@packages/loot-core/src/mocks/budget.ts`:
- Around line 746-747: Replace the direct console.error call with the project's
logger (e.g., logger.error or getLogger().error) so the mock uses the
centralized logging API; specifically, in the block that currently does
console.error('Unknown account name for test budget: ', account.name) (near the
fillChecking(app, account, payees, allGroups) call), import or obtain the logger
instance at the top of the file and call logger.error with the same message and
account.name (or interpolate) to satisfy the actual/prefer-logger-over-console
rule while keeping behavior unchanged.
- Around line 300-303: Replace uses of
app.runHandler('transactions-batch-update', {...}) with the direct handler call
style app['transactions-batch-update']({...}) for consistency with other
helpers; locate occurrences of app.runHandler('transactions-batch-update', ...)
(including the similar calls later in the file) and change them to the bracketed
handler invocation (app['transactions-batch-update'](...)) only if that handler
exists on the app type, otherwise leave runHandler if this kebab-case
indirection is intentionally required for incremental migration.
In `@packages/loot-core/src/platform/client/connection/index-types.ts`:
- Around line 7-15: The proxy type ServerProxy currently makes the handler
argument optional and doesn't force a Promise result; update ServerProxy so each
method preserves the handler parameter list exactly (use Parameters<Handlers[K]>
to keep the same parameter tuple instead of a single optional args) and make the
return type explicitly promise-returning (wrap/await the handler's ReturnType so
server.* methods return Promise of the handler result). Apply the same corrected
signature to the declared server and the init() return type so server and init()
align with the transport contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8b9b88cb-e914-4c87-85cf-d3afcbbded00
📒 Files selected for processing (33)
packages/desktop-client/src/components/ManageRules.tsxpackages/desktop-client/src/components/mobile/payees/MobilePayeeEditPage.tsxpackages/desktop-client/src/components/mobile/payees/MobilePayeesPage.tsxpackages/desktop-client/src/components/modals/MergeUnusedPayeesModal.tsxpackages/desktop-client/src/components/payees/ManagePayeesWithData.tsxpackages/desktop-client/src/payees/location-adapters.tspackages/desktop-client/src/payees/mutations.tspackages/desktop-client/src/payees/queries.tspackages/loot-core/src/mocks/budget.tspackages/loot-core/src/platform/client/connection/index-types.tspackages/loot-core/src/platform/client/connection/index.browser.tspackages/loot-core/src/platform/client/connection/index.tspackages/loot-core/src/platform/server/connection/index-types.tspackages/loot-core/src/platform/server/connection/index.electron.tspackages/loot-core/src/platform/server/connection/index.tspackages/loot-core/src/server/accounts/app.tspackages/loot-core/src/server/api.tspackages/loot-core/src/server/app.tspackages/loot-core/src/server/budgetfiles/app.tspackages/loot-core/src/server/importers/actual.tspackages/loot-core/src/server/importers/index.tspackages/loot-core/src/server/importers/ynab4.tspackages/loot-core/src/server/importers/ynab5.tspackages/loot-core/src/server/main-app.tspackages/loot-core/src/server/main.test.tspackages/loot-core/src/server/main.tspackages/loot-core/src/server/mutators.tspackages/loot-core/src/server/payees/app.tspackages/loot-core/src/server/sync/index.tspackages/loot-core/src/server/util/budget-name.tspackages/loot-core/src/types/app.tspackages/loot-core/src/types/handlers.tspackages/loot-core/src/types/server-handlers.ts
💤 Files with no reviewable changes (2)
- packages/loot-core/src/server/main-app.ts
- packages/loot-core/src/types/server-handlers.ts
Comment on lines
+102
to
107
| export const init: T.Init = function () { | ||
| if (!initPromise) { | ||
| initPromise = new Promise(connectSocket); | ||
| } | ||
| return initPromise; | ||
| }; |
Contributor
There was a problem hiding this comment.
init() is now idempotent, but clearServer() doesn't reset initPromise.
The caching pattern is good, but if clearServer() is called (lines 171-175), a subsequent init() will return the stale promise pointing to the closed socket. Consider resetting initPromise in clearServer().
Proposed fix
export const clearServer: T.ClearServer = async function () {
if (socketClient != null) {
- return new Promise(closeSocket);
+ await new Promise(closeSocket);
+ initPromise = null;
}
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/loot-core/src/platform/client/connection/index.ts` around lines 102
- 107, init() currently caches the connection promise in initPromise but
clearServer() doesn't reset it, so calling clearServer() leaves initPromise
pointing to a stale/closed socket; update clearServer() to set initPromise =
undefined (or null) after closing/clearing the socket and any related state so
that the next call to init() will call new Promise(connectSocket) again,
referencing the symbols init, clearServer, initPromise, and connectSocket to
locate and modify the code.
Contributor
|
🤖 Auto-generated Release Notes Hey @joel-jeremy! I've automatically created a release notes file based on CodeRabbit's analysis: Category: Enhancements If you're happy with this release note, you can add it to your pull request. If not, you'll need to add your own before a maintainer can review your change. |
✅ Deploy Preview for actualbudget-website ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
This comment has been minimized.
This comment has been minimized.
Contributor
|
VRT tests ❌ failed. View the test report. To update the VRT screenshots, comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/loot-core/src/server/api.ts (1)
709-741: Inconsistent handler invocation style during migration.Some handlers use dot notation with camelCase method names:
mainApp.getCommonPayees()mainApp.getPayees()mainApp.createPayee()mainApp.batchChangePayees()mainApp.mergePayees()While others use bracket notation with kebab-case:
mainApp['rules-get']()mainApp['budget/budget-amount']()This inconsistency is expected given the PR description mentions incremental migration to camelCase. Consider adding a comment noting this is transitional, or track which handlers still need renaming.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/loot-core/src/server/api.ts` around lines 709 - 741, Add a short transitional comment above this handlers block stating that handler invocation styles are being migrated from bracket/kebab-case to dot/camelCase and that mixed usage is intentional for now; reference the specific handlers to clarify scope (e.g., handlers['api/payees-get'], handlers['api/payee-create'], handlers['api/payees-merge'] and the still-kebab callers like mainApp['rules-get'] and mainApp['budget/budget-amount']), and add a TODO with a tracking ticket or label indicating remaining functions to rename (e.g., list of kebab-case handlers) so future PRs can complete the migration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/loot-core/src/server/api.ts`:
- Line 1027: The test still imports and calls the removed installAPI; update it
to import the new app export instead (import { app } from './api') and use app
directly in assertions (e.g., call the request/handler entry points on app or
reference app.handlers if the test expects the handlers object). If the test
needs to construct the app with a custom ServerHandlers, import and call
createApp(ServerHandlers) instead (use the createApp symbol) or adjust
expectations to use the already-created app; replace any use of installAPI(...)
with one of these approaches.
---
Nitpick comments:
In `@packages/loot-core/src/server/api.ts`:
- Around line 709-741: Add a short transitional comment above this handlers
block stating that handler invocation styles are being migrated from
bracket/kebab-case to dot/camelCase and that mixed usage is intentional for now;
reference the specific handlers to clarify scope (e.g.,
handlers['api/payees-get'], handlers['api/payee-create'],
handlers['api/payees-merge'] and the still-kebab callers like
mainApp['rules-get'] and mainApp['budget/budget-amount']), and add a TODO with a
tracking ticket or label indicating remaining functions to rename (e.g., list of
kebab-case handlers) so future PRs can complete the migration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9fe4e024-acbf-44d3-9062-f05e42750a0c
📒 Files selected for processing (4)
packages/loot-core/src/server/api.tspackages/loot-core/src/server/main.tspackages/loot-core/src/types/handlers.tsupcoming-release-notes/7246.md
✅ Files skipped from review due to trivial changes (1)
- upcoming-release-notes/7246.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/loot-core/src/types/handlers.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/loot-core/src/types/handlers.ts (1)
45-45: Consider removing redundant{} &prefix.Intersecting with an empty object type (
{}) has no effect on the resulting type. This appears to be unnecessary.♻️ Proposed simplification
-export type Handlers = {} & ServerHandlers & ApiHandlers; +export type Handlers = ServerHandlers & ApiHandlers;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/loot-core/src/types/handlers.ts` at line 45, The type alias Handlers currently intersects with an empty object ("export type Handlers = {} & ServerHandlers & ApiHandlers;"); remove the redundant "{} &" so the declaration becomes a direct intersection of ServerHandlers and ApiHandlers (i.e., change Handlers to use ServerHandlers & ApiHandlers) to simplify the type without changing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/loot-core/src/server/api.test.ts`:
- Around line 21-27: The test's mock of apiApp['accounts-bank-sync'] won't
intercept the internal call from the api/bank-sync handler because that handler
uses a different app instance; update the test to mock the exact dependency the
handler calls (or provide the same app instance to the handler). Concretely,
either (a) ensure the handler under test receives the same apiApp object you set
the mock on (pass that instance into the api/bank-sync handler or factory), or
(b) vi.mock or stub the module/function the handler invokes for account sync
(the real implementation called by "api/bank-sync") so your mock of
"accounts-bank-sync" is the one actually executed; adjust the test to call the
handler with the injected instance or mocked module so the rejection path
returns { errors: ['connection-failed'] } and the expect(...).rejects.toThrow
assertion runs against that behavior.
- Around line 12-17: The test currently mocks apiApp['accounts-bank-sync'] which
is never called for single-account sync; instead mock mainApp['bank-sync'] (the
handler invoked by api/bank-sync when accountId is provided) by replacing the
apiApp mock with a vi.fn().mockResolvedValue({ errors: [] }) on
mainApp['bank-sync'], call apiApp['api/bank-sync']({ accountId: 'account1' }),
and assert that mainApp['bank-sync'] was called with the expected single-account
argument; alternatively, refactor tests to inject or share the same app instance
from createApp so handler mocks on apiApp affect calls inside the handler.
---
Nitpick comments:
In `@packages/loot-core/src/types/handlers.ts`:
- Line 45: The type alias Handlers currently intersects with an empty object
("export type Handlers = {} & ServerHandlers & ApiHandlers;"); remove the
redundant "{} &" so the declaration becomes a direct intersection of
ServerHandlers and ApiHandlers (i.e., change Handlers to use ServerHandlers &
ApiHandlers) to simplify the type without changing behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6eb4799f-14ac-4871-bd8c-3f76b1898cef
📒 Files selected for processing (3)
packages/loot-core/src/server/api.test.tspackages/loot-core/src/server/main.tspackages/loot-core/src/types/handlers.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/loot-core/src/server/main.ts
Member
|
Really cool. WDYT about starting to use the API directly already? I think it should be possible with this PR: #7247 (spoilers: I have not properly tested though and won't have time in the next week.. but theoretically it should work). |
Member
Author
That would be nice! I could adopt this PR and remove the desktop-client changes since we're going to go through api. Once we merge your PR, I can make a follow PR to update api to use the server proxy added on this PR and remove the send calls (possibly remove the manual translation on the methods.ts file) |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/loot-core/src/server/api.test.ts`:
- Around line 26-36: The test contains an if without braces inside the mocked
implementation for mainApp.runHandler; update the anonymous async function in
the vi.spyOn(...).mockImplementation to wrap the if body in curly braces (i.e.,
add { return { errors: [...] }; } for the branch where name ===
'accounts-bank-sync') so the code conforms to the ESLint `curly` rule while
leaving the thrown Error for unexpected handlers and the apiApp['api/bank-sync']
assertion unchanged.
- Around line 12-22: The test currently creates a spy on mainApp.runHandler
inline and uses it directly in expect, and also has an if without braces in the
mockImplementation; fix by assigning the spy to a local variable (e.g., const
runHandlerSpy = vi.spyOn(mainApp, 'runHandler')...) and use that variable in the
assertion to avoid the unbound-method lint error, and add curly braces around
the if body inside the mockImplementation for mainApp.runHandler to satisfy the
missing-braces rule; keep the mocked behavior (returning { errors: [] } for name
=== 'accounts-bank-sync' and throwing for others) and the call to
apiApp['api/bank-sync']({ accountId: 'account1' }) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ca8f8091-f25c-44b6-8599-c2b8f1b74bd0
📒 Files selected for processing (3)
packages/loot-core/src/server/accounts/app-bank-sync.test.tspackages/loot-core/src/server/api.test.tspackages/loot-core/src/server/api.ts
✅ Files skipped from review due to trivial changes (1)
- packages/loot-core/src/server/api.ts
Comment on lines
12
to
22
| vi.spyOn(mainApp, 'runHandler').mockImplementation( | ||
| async (name: string) => { | ||
| if (name === 'accounts-bank-sync') return { errors: [] }; | ||
| throw new Error(`Unexpected handler: ${name}`); | ||
| }, | ||
| ); | ||
|
|
||
| await handlers['api/bank-sync']({ accountId: 'account1' }); | ||
| expect(handlers['accounts-bank-sync']).toHaveBeenCalledWith({ | ||
| await apiApp['api/bank-sync']({ accountId: 'account1' }); | ||
| expect(mainApp.runHandler).toHaveBeenCalledWith('accounts-bank-sync', { | ||
| ids: ['account1'], | ||
| }); |
Contributor
There was a problem hiding this comment.
Fix lint violations: unbound-method and missing curly braces.
Static analysis flags two issues:
- Line 20:
unbound-method- referencingmainApp.runHandlerin the assertion can cause scoping issues - Line 14: Missing curly braces after
ifcondition
Store the spy in a variable to fix the unbound-method issue and add braces to the if statement.
🔧 Proposed fix
it('should sync a single account when accountId is provided', async () => {
- vi.spyOn(mainApp, 'runHandler').mockImplementation(
+ const runHandlerSpy = vi.spyOn(mainApp, 'runHandler').mockImplementation(
async (name: string) => {
- if (name === 'accounts-bank-sync') return { errors: [] };
+ if (name === 'accounts-bank-sync') {
+ return { errors: [] };
+ }
throw new Error(`Unexpected handler: ${name}`);
},
);
await apiApp['api/bank-sync']({ accountId: 'account1' });
- expect(mainApp.runHandler).toHaveBeenCalledWith('accounts-bank-sync', {
+ expect(runHandlerSpy).toHaveBeenCalledWith('accounts-bank-sync', {
ids: ['account1'],
});
});🧰 Tools
🪛 GitHub Check: autofix
[failure] 20-20: typescript-eslint(unbound-method)
Avoid referencing unbound methods which may cause unintentional scoping of this.
🪛 GitHub Check: lint
[failure] 20-20: typescript-eslint(unbound-method)
Avoid referencing unbound methods which may cause unintentional scoping of this.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/loot-core/src/server/api.test.ts` around lines 12 - 22, The test
currently creates a spy on mainApp.runHandler inline and uses it directly in
expect, and also has an if without braces in the mockImplementation; fix by
assigning the spy to a local variable (e.g., const runHandlerSpy =
vi.spyOn(mainApp, 'runHandler')...) and use that variable in the assertion to
avoid the unbound-method lint error, and add curly braces around the if body
inside the mockImplementation for mainApp.runHandler to satisfy the
missing-braces rule; keep the mocked behavior (returning { errors: [] } for name
=== 'accounts-bank-sync' and throwing for others) and the call to
apiApp['api/bank-sync']({ accountId: 'account1' }) unchanged.
Comment on lines
+26
to
+36
| vi.spyOn(mainApp, 'runHandler').mockImplementation( | ||
| async (name: string) => { | ||
| if (name === 'accounts-bank-sync') | ||
| return { errors: [{ message: 'connection-failed' }] }; | ||
| throw new Error(`Unexpected handler: ${name}`); | ||
| }, | ||
| ); | ||
|
|
||
| await expect( | ||
| handlers['api/bank-sync']({ accountId: 'account2' }), | ||
| ).rejects.toThrow('Bank sync error: connection-failed'); | ||
|
|
||
| expect(getBankSyncError).toHaveBeenCalledWith('connection-failed'); | ||
| apiApp['api/bank-sync']({ accountId: 'account2' }), | ||
| ).rejects.toThrow('connection-failed'); |
Contributor
There was a problem hiding this comment.
Same lint violation: missing curly braces after if condition.
Line 29 violates the ESLint curly rule. Add braces around the if body.
🔧 Proposed fix
it('should throw an error when bank sync fails', async () => {
vi.spyOn(mainApp, 'runHandler').mockImplementation(
async (name: string) => {
- if (name === 'accounts-bank-sync')
- return { errors: [{ message: 'connection-failed' }] };
+ if (name === 'accounts-bank-sync') {
+ return { errors: [{ message: 'connection-failed' }] };
+ }
throw new Error(`Unexpected handler: ${name}`);
},
);📝 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.
Suggested change
| vi.spyOn(mainApp, 'runHandler').mockImplementation( | |
| async (name: string) => { | |
| if (name === 'accounts-bank-sync') | |
| return { errors: [{ message: 'connection-failed' }] }; | |
| throw new Error(`Unexpected handler: ${name}`); | |
| }, | |
| ); | |
| await expect( | |
| handlers['api/bank-sync']({ accountId: 'account2' }), | |
| ).rejects.toThrow('Bank sync error: connection-failed'); | |
| expect(getBankSyncError).toHaveBeenCalledWith('connection-failed'); | |
| apiApp['api/bank-sync']({ accountId: 'account2' }), | |
| ).rejects.toThrow('connection-failed'); | |
| vi.spyOn(mainApp, 'runHandler').mockImplementation( | |
| async (name: string) => { | |
| if (name === 'accounts-bank-sync') { | |
| return { errors: [{ message: 'connection-failed' }] }; | |
| } | |
| throw new Error(`Unexpected handler: ${name}`); | |
| }, | |
| ); | |
| await expect( | |
| apiApp['api/bank-sync']({ accountId: 'account2' }), | |
| ).rejects.toThrow('connection-failed'); |
🧰 Tools
🪛 GitHub Check: lint
[failure] 29-29: eslint(curly)
Expected { after 'if' condition.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/loot-core/src/server/api.test.ts` around lines 26 - 36, The test
contains an if without braces inside the mocked implementation for
mainApp.runHandler; update the anonymous async function in the
vi.spyOn(...).mockImplementation to wrap the if body in curly braces (i.e., add
{ return { errors: [...] }; } for the branch where name ===
'accounts-bank-sync') so the code conforms to the ESLint `curly` rule while
leaving the thrown Error for unexpected handlers and the apiApp['api/bank-sync']
assertion unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Note to reviewer: This is easier to review by commits
Currently, the desktop-client communicates with the loot-core server by calling a generic
sendfunction with a string handler name:This PR introduces a proxy-based
serverobject that makes these calls look like regular method calls:The same pattern is also applied within loot-core itself, so internal code no longer needs to call
runHandlerdirectly.Migration plan
The migration will be done incrementally, one server app at a time — for example, one PR for the budget folder, another for accounts, and so on. This keeps each PR focused and reviewable.
As part of each migration PR, handler names will be renamed from kebab-case to camelCase so the proxy methods are idiomatic JavaScript:
Payees app handlers are already renamed to camelCase on this PR.
This is groundwork for making the api package cross-platform (usable from desktop-client directly), since the proxy surface provides a clean, typed interface without leaking transport details.
Related issue(s)
Testing
Checklist
Bundle Stats
View detailed bundle stats
desktop-client
Total
Changeset
home/runner/work/actual/actual/packages/loot-core/src/platform/client/connection/index.browser.tssrc/components/modals/MergeUnusedPayeesModal.tsxsrc/components/ManageRules.tsxsrc/components/mobile/payees/MobilePayeeEditPage.tsxsrc/payees/mutations.tssrc/components/payees/ManagePayeesWithData.tsxsrc/components/mobile/payees/MobilePayeesPage.tsxsrc/payees/queries.tssrc/payees/location-adapters.tsView detailed bundle breakdown
Added
No assets were added
Removed
No assets were removed
Bigger
No assets were bigger
Smaller
Unchanged
loot-core
Total
Changeset
home/runner/work/actual/actual/packages/loot-core/src/server/app.tshome/runner/work/actual/actual/packages/loot-core/src/server/mutators.tshome/runner/work/actual/actual/packages/loot-core/src/server/importers/ynab4.tshome/runner/work/actual/actual/packages/loot-core/src/server/sync/index.tshome/runner/work/actual/actual/packages/loot-core/src/server/importers/ynab5.tshome/runner/work/actual/actual/packages/loot-core/src/platform/server/fs/index.tshome/runner/work/actual/actual/packages/loot-core/src/server/schedules/app.tshome/runner/work/actual/actual/packages/loot-core/src/server/undo.tshome/runner/work/actual/actual/packages/loot-core/src/server/budgetfiles/backups.tshome/runner/work/actual/actual/packages/loot-core/src/server/transactions/index.tshome/runner/work/actual/actual/packages/loot-core/src/server/budgetfiles/app.tshome/runner/work/actual/actual/packages/loot-core/src/server/accounts/app.tshome/runner/work/actual/actual/packages/loot-core/src/server/util/budget-name.tshome/runner/work/actual/actual/packages/loot-core/src/server/sync/reset.tshome/runner/work/actual/actual/packages/loot-core/src/platform/server/connection/index.tshome/runner/work/actual/actual/packages/loot-core/src/server/importers/index.tshome/runner/work/actual/actual/packages/loot-core/src/server/importers/actual.tshome/runner/work/actual/actual/packages/loot-core/src/platform/server/fetch/index.tshome/runner/work/actual/actual/packages/loot-core/src/mocks/budget.tshome/runner/work/actual/actual/packages/loot-core/src/server/api.tshome/runner/work/actual/actual/packages/loot-core/src/server/main.tshome/runner/work/actual/actual/packages/loot-core/src/server/payees/app.tshome/runner/work/actual/actual/packages/loot-core/src/server/main-app.tsView detailed bundle breakdown
Added
Removed
Bigger
No assets were bigger
Smaller
No assets were smaller
Unchanged
No assets were unchanged
api
Total
Changeset
home/runner/work/actual/actual/packages/loot-core/src/server/app.tshome/runner/work/actual/actual/packages/loot-core/src/server/mutators.tshome/runner/work/actual/actual/packages/loot-core/src/server/main.tshome/runner/work/actual/actual/packages/loot-core/src/server/importers/ynab4.tshome/runner/work/actual/actual/packages/loot-core/src/server/sync/index.tshome/runner/work/actual/actual/packages/loot-core/src/server/accounts/app.tshome/runner/work/actual/actual/packages/loot-core/src/server/budgetfiles/app.tshome/runner/work/actual/actual/packages/loot-core/src/server/importers/ynab5.tshome/runner/work/actual/actual/packages/loot-core/src/server/util/budget-name.tshome/runner/work/actual/actual/packages/loot-core/src/server/importers/index.tshome/runner/work/actual/actual/packages/loot-core/src/server/importers/actual.tshome/runner/work/actual/actual/packages/loot-core/src/mocks/budget.tshome/runner/work/actual/actual/packages/loot-core/src/server/api.tshome/runner/work/actual/actual/packages/loot-core/src/server/payees/app.tshome/runner/work/actual/actual/packages/loot-core/src/server/main-app.tsView detailed bundle breakdown
Added
No assets were added
Removed
No assets were removed
Bigger
No assets were bigger
Smaller
Unchanged
cli
Total
View detailed bundle breakdown
Added
No assets were added
Removed
No assets were removed
Bigger
No assets were bigger
Smaller
No assets were smaller
Unchanged