Handling tunnel errors as network failure #3697
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 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
|
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 |
WalkthroughThe changes in the pull request focus on enhancing the error handling mechanisms within the The existing error handling for non-200 responses is maintained, which includes specific handling for 500 status codes and JSON parsing errors. The overall structure of the 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 1
🧹 Outside diff range and nitpick comments (2)
packages/loot-core/src/server/post.ts (2)
28-30: Consider using a more specific error message for tunnel errorsWhile treating tunnel errors as network failures is a good approach, using a more specific error message could provide better context for debugging and user communication.
Consider modifying the error message to be more specific:
-throw new PostError('network-failure'); +throw new PostError('tunnel-network-failure');This change would make it easier to distinguish between general network failures and those specifically related to tunnel issues.
Line range hint
65-69: Handle potential JSON parsing errors in thepostfunctionWhile reviewing the changes, I noticed that the
postfunction, which uses the modifiedthrowIfNot200, doesn't handle potential JSON parsing errors explicitly.Consider wrapping the JSON parsing in a try-catch block to handle parsing errors gracefully:
- res = JSON.parse(text); + try { + res = JSON.parse(text); + } catch (err) { + throw new PostError('invalid-json-response', { meta: text }); + }This change would provide more specific error handling for invalid JSON responses, improving the overall robustness of the function.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3697.mdis excluded by!**/*.md
📒 Files selected for processing (1)
- packages/loot-core/src/server/post.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
packages/loot-core/src/server/post.ts (1)
19-31: Robust implementation for handling tunnel errorsThe implementation effectively addresses the PR objectives by introducing tunnel error detection. It improves error handling for sync servers behind tunnels, which aligns with the goal of providing clearer communication about server availability.
The overall approach is sound and meets the requirements. The code is well-commented and logically structured. With the minor suggestions provided in the other comments, this implementation will significantly enhance the user experience when dealing with tunnel-related issues.
Why?
If the sync server is behind an active tunnel but the server is offline we get a error message like:
We get the error because the tunnel service says it can't connect to the server and we're not handling it.
This could be a common situation when the user wants to run the sync server on a computer in their house, but expose it to the internet for mobile use. Tunneling services like ngrok stay online forever, but If they turn their computer off the sync server goes offline.
This fix detects known tunnels (ngrok for now) and if the tunnel is telling us the server is offline we handle it as normal - we tell the user it's offline.
This was found while working on the desktop app server sync: #3631