[Typescript] Server event types #4110
Conversation
actual-github-bot
bot
changed the title
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Bundle Stats — desktop-clientHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged
|
Bundle Stats — loot-coreHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller
Unchanged No assets were unchanged |
| @@ -117,8 +117,8 @@ function SyncButton({ style, isMobile = false }: SyncButtonProps) { | |||
| >(null); | |||
|
|
|||
| useEffect(() => { | |||
| const unlisten = listen('sync-event', ({ type, subtype, syncDisabled }) => { | |||
| if (type === 'start') { | |||
| const unlisten = listen('sync-event', event => { | |||
There was a problem hiding this comment.
Changed to event so typescript can infer discrimanted type better vs destructured.
joel-jeremy
changed the title
WalkthroughThe pull request introduces a series of modifications across multiple files in the desktop client and core packages, focusing on streamlining event handling and improving type safety. The changes primarily involve simplifying event listener parameter handling by transitioning from destructured parameters to using a single In the Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/loot-core/src/types/server-events.d.ts (1)
46-68: Consider documenting event types.While the type definitions are clear, adding JSDoc comments for each event type would improve maintainability by documenting the purpose and usage of each event.
Example:
+/** Event emitted when backups are updated */ type BackupUpdatedEvent = Backup[]; +/** Event emitted when cell values change */ type CellsChangedEvent = Array<{ name: string; value: string | number | boolean; }>;packages/desktop-client/src/global-events.ts (1)
30-40: Consider using optional chaining for better null safety.The code accesses nested properties without null checks. Consider using optional chaining to make the code more robust.
- if (prefs && prefs.id) { + if (prefs?.id) {🧰 Tools
🪛 Biome (1.9.4)
[error] 35-35: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4110.mdis excluded by!**/*.md
📒 Files selected for processing (15)
packages/desktop-client/src/components/Titlebar.tsx(2 hunks)packages/desktop-client/src/components/budget/index.tsx(1 hunks)packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx(1 hunks)packages/desktop-client/src/components/mobile/budget/CategoryTransactions.tsx(1 hunks)packages/desktop-client/src/components/mobile/budget/index.tsx(1 hunks)packages/desktop-client/src/components/payees/ManagePayeesWithData.tsx(1 hunks)packages/desktop-client/src/global-events.ts(1 hunks)packages/loot-core/src/client/query-helpers.ts(3 hunks)packages/loot-core/src/client/shared-listeners.ts(7 hunks)packages/loot-core/src/platform/server/connection/index.d.ts(1 hunks)packages/loot-core/src/server/accounts/transactions.ts(2 hunks)packages/loot-core/src/server/app.ts(1 hunks)packages/loot-core/src/server/main-app.ts(1 hunks)packages/loot-core/src/server/schedules/app.ts(2 hunks)packages/loot-core/src/types/server-events.d.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/loot-core/src/server/main-app.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/desktop-client/src/global-events.ts
[error] 35-35: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ 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 (18)
packages/loot-core/src/platform/server/connection/index.d.ts (1)
12-12: LGTM! Fixed case-sensitivity in generic type parameter.The change from
ServerEvents[k]toServerEvents[K]correctly matches the generic type parameterKdefined in the extends clause, fixing a potential type error.packages/loot-core/src/server/app.ts (2)
12-15: LGTM! Well-structured event type definitions.The
Eventstype alias provides clear type definitions for sync and load-budget events, enhancing type safety.
18-18: LGTM! Improved type safety for events property.The events property is now properly typed with
Emitter<Events>, ensuring type-safe event handling.packages/loot-core/src/types/server-events.d.ts (3)
4-18: LGTM! Comprehensive sync state definitions.The
SyncSubtypetype provides a clear and extensible list of sync states. The inclusion ofstringtype allows for future additions without breaking changes.
20-44: LGTM! Well-structured sync event type definitions.The
SyncEventtype effectively uses discriminated unions to define different sync events with their specific payloads. The inclusion of optionalmetafield provides flexibility for additional context.
71-85: LGTM! Consistent event interface updates.The
ServerEventsinterface effectively uses the newly defined types, providing a clear contract for event handling throughout the application.packages/desktop-client/src/components/mobile/budget/CategoryTransactions.tsx (1)
65-67: LGTM! Improved event handling pattern.The change to use a single
eventparameter instead of destructuring improves type safety by leveraging the newly definedSyncEventtype. This change maintains the same functionality while making the code more maintainable.packages/desktop-client/src/global-events.ts (1)
24-25: LGTM! Event handler simplified.The simplified event handler improves code clarity by removing unused parameters.
packages/desktop-client/src/components/payees/ManagePayeesWithData.tsx (1)
52-54: LGTM! Improved TypeScript type inference.The change to use direct event property access allows TypeScript to better infer discriminated union types.
packages/loot-core/src/server/accounts/transactions.ts (1)
4-4: LGTM! Enhanced type safety with explicit types.The addition of PayeeEntity import and explicit Set type annotation improves type safety and code maintainability.
Also applies to: 58-58
packages/desktop-client/src/components/Titlebar.tsx (1)
Line range hint
120-145: LGTM! Improved TypeScript type inference for sync events.The changes to use direct event property access consistently improve TypeScript's ability to infer discriminated union types, making the code more type-safe.
packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx (1)
270-272: LGTM! Clean event handling pattern.The change to use a single
eventparameter improves type safety and follows TypeScript best practices for event handling.packages/loot-core/src/client/shared-listeners.ts (1)
11-11: LGTM! Consistent event handling pattern.The changes standardize event parameter naming and access patterns across the codebase, improving type safety and maintainability while preserving the existing error handling and notification logic.
Also applies to: 18-18, 29-30, 50-50, 58-58, 257-257, 268-268, 292-292
packages/loot-core/src/client/query-helpers.ts (1)
65-65: Great type safety improvements!The changes enhance type safety by:
- Restricting
_supportedSyncTypesto specific literal types ('applied' | 'success').- Ensuring type-safe event handling in the sync event listener.
This helps catch potential errors at compile time and makes the code more maintainable.
Also applies to: 110-111, 165-165, 172-176
packages/desktop-client/src/components/budget/index.tsx (1)
111-120: LGTM! Consistent with codebase-wide improvements.The changes align with the standardized event handling pattern seen across other files, improving type safety while maintaining the existing functionality.
packages/desktop-client/src/components/mobile/budget/index.tsx (1)
72-82: LGTM! Event handling implementation looks good.The changes improve type safety by accessing properties directly from the event object instead of destructuring. The logic for checking sync success and table updates remains intact.
packages/loot-core/src/server/schedules/app.ts (2)
544-545: LGTM! Improved type safety for syncDisabled.Changed
syncDisabledfrom string'false'to booleanfalse, which is the correct type for this property.
536-536: Verify client-side handling of schedules-offline event.The removal of the
payeesarray from the event payload might affect client components that expect this data.Let's verify the client-side handling:
|
/update-vrt |
joel-jeremy
force-pushed
the
server-event-types
branch
from
31b4cb4 to
7e8bb62
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
packages/loot-core/src/server/app.ts (1)
1-1: Consider enabling TypeScript strict mode.The
@ts-strict-ignoredirective disables TypeScript's strict mode for the entire file. Consider addressing the type issues and enabling strict mode for better type safety.packages/loot-core/src/types/server-events.d.ts (3)
4-18: Consider documenting sync subtypes and making the type more strictThe
SyncSubtypetype includes well-named literals for various sync states, but lacks documentation explaining each case. Additionally, the catch-allstringtype at the end makes the type less strict, potentially hiding typos or invalid states.Consider:
- Adding JSDoc comments to document each sync subtype
- Removing the catch-all
stringtype unless there's a specific need for extensibility+/** Represents different synchronization states and error conditions */ type SyncSubtype = + /** Data is out of sync with the server */ | 'out-of-sync' + /** Failed to apply sync changes */ | 'apply-failure' // ... add docs for other subtypes - | string;
20-44: Improve type safety of meta and data propertiesThe
SyncEventtype uses loose types formetaanddataproperties which could be made more type-safe.Consider:
- Using a more specific type for
metainstead ofRecord<string, unknown>- Creating specific types for
dataandprevDataMaps based on the table typestype SyncEvent = { - meta?: Record<string, unknown>; + meta?: { + timestamp?: number; + // Add other known meta properties + }; } & ( | { type: 'applied'; tables: string[]; - data?: Map<string, unknown>; - prevData?: Map<string, unknown>; + data?: Map<string, TableData>; + prevData?: Map<string, TableData>; } // ... ); +/** Represents the structure of table data */ +type TableData = { + // Define known table data structure +};
53-56: Consider using Record<never, never> for events without dataSeveral event types are defined as
undefined, which works but isn't as explicit as it could be about the intent.Consider using
Record<never, never>or an empty object type{}to explicitly indicate these events don't carry data:-type FallbackWriteErrorEvent = undefined; +type FallbackWriteErrorEvent = Record<never, never>; -type FinishImportEvent = undefined; +type FinishImportEvent = Record<never, never>; // Apply similar changes to other undefined event typesThis makes it more explicit that these are events that intentionally carry no data, rather than potentially undefined events.
Also applies to: 62-68
packages/desktop-client/src/global-events.ts (2)
Line range hint
30-41: Use optional chaining for safer property accessThe code could benefit from using optional chaining to safely access the
prefs.idproperty.- if (prefs && prefs.id) { + if (prefs?.id) { if (event.type === 'applied') { const tables = event.tables; if (tables.includes('payees') || tables.includes('payee_mapping')) {🧰 Tools
🪛 Biome (1.9.4)
[error] 35-35: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
Line range hint
30-41: Consider consolidating sync event handlersThere are two separate handlers for the 'sync-event' type. This could lead to confusion about the order of execution and make the code harder to maintain.
Consider consolidating the handlers into a single function:
- listen('sync-event', async event => { - const prefs = store.getState().prefs.local; - if (prefs && prefs.id) { - if (event.type === 'applied') { - const tables = event.tables; - if (tables.includes('payees') || tables.includes('payee_mapping')) { - actions.getPayees(); - } - } - } - }); - - listen('sync-event', async ({ type }) => { - if (type === 'unauthorized') { - actions.addNotification({ - type: 'warning', - message: 'Unable to authenticate with server', - sticky: true, - id: 'auth-issue', - }); - } - }); + listen('sync-event', async event => { + const prefs = store.getState().prefs.local; + + if (event.type === 'unauthorized') { + actions.addNotification({ + type: 'warning', + message: 'Unable to authenticate with server', + sticky: true, + id: 'auth-issue', + }); + return; + } + + if (prefs?.id && event.type === 'applied') { + const tables = event.tables; + if (tables.includes('payees') || tables.includes('payee_mapping')) { + actions.getPayees(); + } + } + });Also applies to: 44-55
🧰 Tools
🪛 Biome (1.9.4)
[error] 35-35: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4110.mdis excluded by!**/*.md
📒 Files selected for processing (15)
packages/desktop-client/src/components/Titlebar.tsx(2 hunks)packages/desktop-client/src/components/budget/index.tsx(1 hunks)packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx(1 hunks)packages/desktop-client/src/components/mobile/budget/CategoryTransactions.tsx(1 hunks)packages/desktop-client/src/components/mobile/budget/index.tsx(1 hunks)packages/desktop-client/src/components/payees/ManagePayeesWithData.tsx(1 hunks)packages/desktop-client/src/global-events.ts(1 hunks)packages/loot-core/src/client/query-helpers.ts(3 hunks)packages/loot-core/src/client/shared-listeners.ts(7 hunks)packages/loot-core/src/platform/server/connection/index.d.ts(1 hunks)packages/loot-core/src/server/accounts/transactions.ts(2 hunks)packages/loot-core/src/server/app.ts(1 hunks)packages/loot-core/src/server/main-app.ts(1 hunks)packages/loot-core/src/server/schedules/app.ts(2 hunks)packages/loot-core/src/types/server-events.d.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- packages/desktop-client/src/components/mobile/budget/index.tsx
- packages/loot-core/src/server/main-app.ts
- packages/desktop-client/src/components/payees/ManagePayeesWithData.tsx
- packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx
- packages/loot-core/src/client/shared-listeners.ts
- packages/desktop-client/src/components/mobile/budget/CategoryTransactions.tsx
- packages/desktop-client/src/components/Titlebar.tsx
- packages/loot-core/src/server/accounts/transactions.ts
- packages/loot-core/src/platform/server/connection/index.d.ts
- packages/loot-core/src/server/schedules/app.ts
- packages/desktop-client/src/components/budget/index.tsx
- packages/loot-core/src/client/query-helpers.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/desktop-client/src/global-events.ts
[error] 35-35: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Visual regression
- GitHub Check: Functional
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (5)
packages/loot-core/src/server/app.ts (3)
2-2: LGTM! Import changes improve type safety.The addition of the
Emittertype import andServerEventsimport enhances type safety for event handling.Also applies to: 5-5
12-15: LGTM! Well-structured event type definitions.The
Eventstype provides clear typing for both server events and application-specific events, improving type safety and maintainability.
18-18: Verify event handler implementations match the new type.The
eventsproperty type is now properly constrained withEmitter<Events>. Let's verify that all event handlers in the codebase conform to these types.✅ Verification successful
Event handler implementations verified and type-safe
All event handler implementations in the codebase correctly conform to the
Emitter<Events>type constraint. Both 'sync' and 'load-budget' events are used with proper type signatures throughout the application.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for event handler implementations to verify type conformance rg -t typescript "events\.(on|emit)\('(sync|load-budget)" --no-headingLength of output: 110
Script:
#!/bin/bash # Search for all event handler implementations in .ts and .tsx files rg "events\.(on|emit)\(" -t ts -t tsx -A 2 # Find Events type definition ast-grep --pattern 'type Events = { $$$ }'Length of output: 367
Script:
#!/bin/bash # Search for event usages with simple file extension rg "events\.(on|emit)\(" --type-add 'ts:*.{ts,tsx}' -t ts -A 2 # Find ServerEvents type definition ast-grep --pattern 'type ServerEvents = { $$$ }' # Search specifically for sync and load-budget event usages rg "(sync-event|load-budget)" --type-add 'ts:*.{ts,tsx}' -t ts -A 2Length of output: 18524
packages/loot-core/src/types/server-events.d.ts (1)
70-86: LGTM! Well-structured event type definitionsThe
ServerEventsinterface is well-organized with consistent use of defined types and follows good practices for event naming.packages/desktop-client/src/global-events.ts (1)
24-26: LGTM! Correctly simplified event handlerThe removal of the unused
payeesparameter aligns with theSchedulesOfflineEventtype definition.
No description provided.