🐛 Fix Initializing the connection to the local database hang #4375
Conversation
✅ 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
Smaller No assets were smaller Unchanged No assets were unchanged |
MikesGlitch
changed the title
MikesGlitch
changed the title
WalkthroughThe changes modify both the client-side and server-side connection mechanisms. On the client side, the 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
🧹 Nitpick comments (1)
packages/loot-core/src/platform/server/connection/index.web.ts (1)
107-119: Consider optimizing retry parameters.The retry mechanism is well-implemented, but the maximum wait time of 100 seconds (500 attempts * 200ms) might be excessive for a local database connection.
Consider reducing
MAX_RECONNECT_ATTEMPTSto a lower value, such as:-const MAX_RECONNECT_ATTEMPTS = 500; +const MAX_RECONNECT_ATTEMPTS = 50; // 10 seconds total wait time
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4375.mdis excluded by!**/*.md
📒 Files selected for processing (2)
packages/loot-core/src/platform/client/fetch/index.browser.ts(1 hunks)packages/loot-core/src/platform/server/connection/index.web.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Functional
- GitHub Check: Visual regression
🔇 Additional comments (3)
packages/loot-core/src/platform/server/connection/index.web.ts (1)
47-51: LGTM! Clear handling of client connection confirmation.The handler correctly stops the retry mechanism when the client confirms connection, effectively resolving the race condition.
packages/loot-core/src/platform/client/fetch/index.browser.ts (2)
99-99: LGTM! Safe handling of potential null messageQueue.The optional chaining operator provides a clean way to handle potential null/undefined values.
104-108: LGTM! Clear signaling of client connection status.The implementation effectively communicates the client's connection status to the backend, enabling proper cancellation of retry attempts.
…udget#4375) * fix initializing to the local db * release notes
It was a race condition. The backend was loading in faster than the frontend. It was sending the connect message to the frontend and the frontend wasn't ready to receive it.
We can't guarantee the order that the frontend or backend will load in, so we have to add retries to deal with it.
Fixes this:
Fixes: #2893