Conversation
vladavoX
commented
Feb 24, 2026
- timeout aproach is not good, we should not block the user when his connections are interupted, or it takes too long to upload.
- check codes and then throw based on them
danixeee
left a comment
There was a problem hiding this comment.
Code Review
Overview
This PR removes the 5-second request timeout from the axios instance and replaces the simple navigator.onLine check with a more robust isLikelyOfflineError() function that detects various network error conditions.
What's Good
- Comprehensive error detection: The
NO_INTERNET_ERROR_CODESset covers common DNS/network failure codes - Defense in depth: Multiple detection strategies (navigator.onLine, error codes, message parsing)
- Clean separation: The
isLikelyOfflineError()helper is well-structured and easy to test
Concerns
-
Complete removal of timeout - This is the main concern. Removing the timeout entirely means requests can hang indefinitely if:
- The server accepts the connection but never responds
- A corporate proxy holds the connection open
- File uploads to a slow endpoint never complete
Suggestion: Consider keeping a longer timeout (e.g., 30-60s for API calls) rather than removing it entirely, or use per-request timeouts for upload operations specifically.
-
navigatoravailability in Node environment - This code runs insrc/node/. Thenavigatorobject is a browser API and won't exist in Node.js. While the code guards against this, it's effectively dead code here. -
Message parsing fragility - String matching on error messages (
'network error','getaddrinfo enotfound', etc.) is fragile—messages can change between axios versions or be localized. Consider relying primarily on error codes. -
Missing error codes - Consider adding:
ETIMEDOUT- connection timeout (especially relevant if you restore timeouts)ECONNREFUSED- server not runningECONNRESET- connection reset by peer
Recommendations
- Don't remove timeout entirely—use a longer default (30-60s) or remove only for upload operations
- Add
ETIMEDOUT,ECONNREFUSED, andECONNRESETtoNO_INTERNET_ERROR_CODES - Add unit tests for
isLikelyOfflineError()with various error scenarios