Update global event listeners to handle sync server and DB update sync events #4161
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
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 No files were changed View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
| @@ -157,6 +158,10 @@ function ErrorFallback({ error }: FallbackProps) { | |||
| } | |||
|
|
|||
| export function App() { | |||
| const store = useStore(); | |||
|
|
|||
| useEffect(() => handleGlobalEvents(store), [store]); | |||
There was a problem hiding this comment.
Moved call to handleGlobalEvents here so that we can have proper cleanup of listeners
| @@ -106,31 +102,6 @@ function BudgetInner(props: BudgetInnerProps) { | |||
| } | |||
|
|
|||
| run(); | |||
|
|
|||
| const unlistens = [ | |||
There was a problem hiding this comment.
No need for this since this is already handled by the global sync-event listener
| @@ -277,10 +276,6 @@ function TransactionListWithPreviews({ | |||
| ) { | |||
| reloadTransactions(); | |||
| } | |||
|
|
|||
| if (tables.includes('payees') || tables.includes('payee_mapping')) { | |||
There was a problem hiding this comment.
No need for this since this is already handled by the global sync-event listener
| @@ -72,10 +71,6 @@ export function CategoryTransactions({ | |||
| ) { | |||
| reloadTransactions(); | |||
| } | |||
|
|
|||
| if (tables.includes('payees') || tables.includes('payee_mapping')) { | |||
There was a problem hiding this comment.
No need for this since this is already handled by the global sync-event listener
| @@ -68,22 +67,6 @@ export function Budget() { | |||
| } | |||
|
|
|||
| init(); | |||
|
|
|||
| const unlisten = listen('sync-event', event => { | |||
There was a problem hiding this comment.
No need for this since this is already handled by the global sync-event listener
| }); | ||
|
|
||
| // TODO: Should this run on mobile too? | ||
| listen('sync-event', async ({ type }) => { |
There was a problem hiding this comment.
Moved this to the shared-listeners.ts file
|
|
||
| // This is experimental: we sync data locally automatically when | ||
| // data changes from the backend | ||
| listen('sync-event', async event => { |
There was a problem hiding this comment.
Moved this to the shared-listeners.ts file. That file has been updated to handle both success and applied events
| @@ -37,9 +36,6 @@ const boundActions = bindActionCreators( | |||
| store.dispatch, | |||
| ); | |||
|
|
|||
| // Listen for global events from the server or main process | |||
| handleGlobalEvents(store); | |||
There was a problem hiding this comment.
Moved inside a useEffect in App.tsx
| const prefs = store.getState().prefs.local; | ||
| if (!prefs || !prefs.id) { | ||
| // Do nothing if no budget is loaded | ||
| return; | ||
| } | ||
|
|
||
| if (event.type === 'success') { | ||
| if (event.type === 'success' || event.type === 'applied') { |
There was a problem hiding this comment.
Refreshes the state whenever a server sync was made (i.e. success type) or a local change was made/applied (i.e. applied type)
| if (tables.includes('payees') || tables.includes('payee_mapping')) { | ||
| if ( | ||
| // Sync on accounts change because so that transfer payees are updated | ||
| tables.includes('accounts') || |
There was a problem hiding this comment.
Main fix for #4082
joel-jeremy
changed the title
joel-jeremy
changed the title
|
Warning Rate limit exceeded@joel-jeremy has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 48 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (11)
WalkthroughThe pull request introduces several changes across multiple files in the Assessment against linked issues
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 (3)
packages/desktop-client/src/redux/index.ts (1)
4-4: LGTM! Consider adding JSDoc comments.The addition of type-safe Redux hooks improves type safety. Consider adding JSDoc comments to document the purpose and usage of these hooks.
+/** + * Type-safe hook for accessing the Redux store. + * @returns The Redux store with proper type inference. + */ export const useStore = useReduxStore.withTypes<AppStore>();Also applies to: 7-11, 13-13
packages/desktop-client/src/global-events.ts (1)
26-37: Consider adding error handling for server errors.The server error notification is generic. Consider providing more specific error information to help users troubleshoot issues.
store.dispatch( addNotification({ type: 'warning', - message: 'Unable to authenticate with server', + message: `Unable to authenticate with server: ${error?.message || 'Unknown error'}`, sticky: true, id: 'auth-issue', + button: { + title: 'Retry', + action: () => store.dispatch(sync()) + } }), );packages/loot-core/src/client/shared-listeners.ts (1)
23-35: Good addition of unauthorized event handling.The unauthorized event handling is well implemented. Consider adding a retry mechanism with exponential backoff for transient authentication issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4161.mdis excluded by!**/*.md
📒 Files selected for processing (11)
eslint.config.mjs(1 hunks)packages/desktop-client/src/components/App.tsx(2 hunks)packages/desktop-client/src/components/budget/index.tsx(2 hunks)packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx(0 hunks)packages/desktop-client/src/components/mobile/budget/CategoryTransactions.tsx(0 hunks)packages/desktop-client/src/components/mobile/budget/index.tsx(1 hunks)packages/desktop-client/src/components/payees/ManagePayeesWithData.tsx(0 hunks)packages/desktop-client/src/global-events.ts(4 hunks)packages/desktop-client/src/index.tsx(0 hunks)packages/desktop-client/src/redux/index.ts(1 hunks)packages/loot-core/src/client/shared-listeners.ts(3 hunks)
💤 Files with no reviewable changes (4)
- packages/desktop-client/src/index.tsx
- packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx
- packages/desktop-client/src/components/payees/ManagePayeesWithData.tsx
- packages/desktop-client/src/components/mobile/budget/CategoryTransactions.tsx
⏰ 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 (7)
packages/desktop-client/src/global-events.ts (1)
22-37: Great improvement in event listener management!The cleanup pattern implementation is excellent. Each listener is properly stored and cleaned up, preventing memory leaks.
Also applies to: 39-40, 43-43, 147-160
packages/desktop-client/src/components/App.tsx (1)
161-163: LGTM! Good placement of global event handling.Moving the global event handling to the App component level and using useEffect for cleanup is a good architectural decision.
packages/loot-core/src/client/shared-listeners.ts (2)
344-347: LGTM! Proper cleanup implementation.The cleanup function ensures proper removal of event listeners.
46-46: Verify the impact of payee updates on transfer payees.The comment indicates that payees are updated when accounts change to handle transfer payees. Ensure this doesn't cause unnecessary updates.
Also applies to: 73-78
✅ Verification successful
Transfer payee updates are necessary and correctly implemented.
The sync is required to maintain data consistency when accounts change, as transfer payees are directly linked to accounts. The implementation includes proper safeguards and only updates when necessary.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for transfer payee usage in the codebase rg -A 5 "transfer.*payee"Length of output: 11193
packages/desktop-client/src/components/budget/index.tsx (1)
89-89: LGTM! Category fetching simplified.The direct approach to fetching categories aligns well with the global event listener pattern, making the code more maintainable.
packages/desktop-client/src/components/mobile/budget/index.tsx (1)
19-19: LGTM! Consistent with desktop implementation.The removal of sync event listeners aligns with the global event handling pattern, maintaining consistency across desktop and mobile components.
eslint.config.mjs (1)
664-669: LGTM! Consistent Redux hook restrictions.The new ESLint rule for
useStorefollows the established pattern for Redux hooks, ensuring consistent usage of custom store implementations across the application.
joel-jeremy
changed the title
|
I did some more digging and there are a lot of other calls to |
joel-jeremy
changed the title
joel-jeremy
force-pushed
the
update-global-event-listeners
branch
from
25a8732 to
1b8cf86
Compare
Fixes #4082
The
global-listenersfile listens to a bunch of events from the backend (i.e. service worker). One of these events is thesync-eventthis event is emitted whenever new messages are received from the sync server (type: 'success') or whenever a DB update is made (type: 'applied'). This change updates thesync-eventhandler so that it handles bothsuccessandappliedsync event types - what this means is that whenever Actual received sync messages from the sync server or whenever a DB update is made, the payees, categories, and accounts in the redux state are automatically updated.This will also help with performance by removing some of the duplicate
sync-eventlisteners resulting in less DB calls