🐛 Fix error modal not showing when in non-secure context #4499
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 No assets were bigger Smaller
Unchanged No assets were unchanged |
MikesGlitch
changed the title
WalkthroughThe pull request introduces changes to enhance error handling during application initialization across several files. In the Possibly related PRs
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/desktop-client/src/browser-server.js (1)
62-62: Log message could be more descriptiveThe log message has been simplified from what was likely more descriptive before.
-console.info('init'); +console.info('Initializing backend...');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4499.mdis excluded by!**/*.md
📒 Files selected for processing (4)
packages/desktop-client/src/browser-server.js(3 hunks)packages/desktop-client/src/components/App.tsx(1 hunks)packages/loot-core/src/platform/client/fetch/index.browser.ts(1 hunks)packages/loot-core/src/platform/server/connection/index.web.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/loot-core/src/platform/server/connection/index.web.ts
⏰ 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 (7)
packages/desktop-client/src/components/App.tsx (1)
130-132: Dependency list reduction looks goodRemoving
cloudFileIdandtfrom the dependencies array is a valid approach to prevent the initialization logic from re-running when these values change, which aligns with the comment about preventing Electron crashes.packages/loot-core/src/platform/client/fetch/index.browser.ts (1)
110-112: Good addition of acknowledgment messageAdding this acknowledgment message when an app-init-failure occurs provides a clear way for the client to signal that it has received the failure message, which pairs well with the retry mechanism in browser-server.js.
packages/desktop-client/src/browser-server.js (5)
39-56: Well-structured retry mechanismThe implementation of constants and a dedicated retry function is a clean approach to solving the race condition mentioned in the PR objectives. The retry interval and maximum attempts are reasonably configured.
72-76: Good application of retry mechanismUsing the retry function here ensures the SharedArrayBuffer error message will be consistently delivered to the client even if it's not immediately ready to receive it.
87-90: Consistent error handling approachThe same retry mechanism is correctly applied to this error case as well, maintaining consistency in the error handling approach.
97-100: Clean interval cleanupAdding logic to clear the retry interval when an acknowledgment is received prevents unnecessary continued retry attempts and potential resource waste.
103-106: Complete error handling coverageThe retry mechanism is appropriately applied to the catch block as well, ensuring all potential initialization failures are handled consistently.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/desktop-client/src/browser-server.js (1)
39-106: Consider resetting reconnectAttempts when starting a new retry sequence.While the implementation works as is, the
reconnectAttemptscounter isn't reset between different failure scenarios. This could lead to reduced retry attempts if multiple failures occur in sequence.const postMessageWithRetry = message => { + reconnectAttempts = 0; const reconnectToClientInterval = setInterval(() => { self.postMessage(message);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/desktop-client/src/browser-server.js(3 hunks)
⏰ 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/browser-server.js (7)
39-41: Constants seem reasonable for retry mechanism.The constants provide a good balance between reliability and performance: 200ms interval with up to 500 attempts gives a maximum retry window of 100 seconds, which should be sufficient for most network conditions and client initialization times.
43-54: Well-implemented retry mechanism with proper termination.The
postMessageWithRetryfunction implements a time-based retry mechanism that will continue attempting to deliver the message to the client until acknowledged. This effectively addresses the race condition mentioned in the PR objectives where the client might not be ready to receive the first message.
56-56: Good approach using a module-scoped variable for interval management.Using a module-scoped variable
appInitFailureIntervalallows for proper cleanup across different code paths.
71-75: Retry mechanism for SharedArrayBuffer error is appropriate.This change addresses the core issue from the PR objectives where the SharedArrayBuffer error screen wasn't consistently displayed for non-HTTPS users.
We need to retry the messages here because can't be sure the client is ready when we send the message. This is only true for this init code because it's the first interaction with the worker. If we get passed this point in the app we know we have the connection.
86-90: Properly handling IDBFailure with retry mechanism.The retry implementation for IDBFailure ensures consistent error message delivery, improving user experience when IndexedDB initialization fails.
96-99: Effective cleanup mechanism for acknowledged failures.This is a critical part of the implementation that stops the retries once the client has acknowledged receipt of the failure message, preventing unnecessary message processing.
102-106: Comprehensive error handling with retry for backend initialization failures.This change ensures that backend initialization errors are consistently communicated to the client, completing the robust error handling system.
The SharedArrayBuffer error screen wasn't always showing for users not using HTTPS - it was the cause of some github issues. The cause was (another) race condition. The problem & fix is very similar to this: #4375
The Fatal error message was the first message we were attempting to send the worker. If the client wasn't ready to receive the message it would just hang on the loading screen. The solution is to add retries.
To replicate:
The old code would hang on the loading screen, the new code will show the Fatal error.
NOTE: I noticed the old code would sometimes work and sometimes not. I found it easier to replicate when devtools was closed.