fix: prevent UI from freezing by adding fetch retries and timout#11
fix: prevent UI from freezing by adding fetch retries and timout#11mhmdrameez wants to merge 1 commit intotinkerhub:developfrom
Conversation
Fixed a bug where API errors were caught silently, causing the UI to freeze or show empty data. Added a retry mechanism, request timeouts, and better error logging.
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/utils/api/fetchData.js (1)
1-43: PR objectives mention retry mechanism, but none is implemented.The PR title states "adding fetch retries" and the objectives explicitly mention "Adds a retry mechanism for fetch requests," but there's no retry logic in this implementation. Only the timeout mechanism was added.
🔎 Example retry implementation
If retry functionality is intended, consider implementing it like this:
export const fetchData = async (maxRetries = 3, retryDelay = 1000) => { const API_URL = process.env.REACT_APP_API_BASE_URL; console.log(API_URL); if (!API_URL) { throw new Error('REACT_APP_API_BASE_URL is not set. Please configure it in your .env file.'); } let lastError; for (let attempt = 0; attempt < maxRetries; attempt++) { try { const controller = new AbortController(); const timeoutId = setTimeout(() => controller.abort(), 10000); try { const response = await fetch(`${API_URL}/checkin/active`, { method: 'GET', headers: { 'Content-Type': 'application/json', }, credentials: 'same-origin', signal: controller.signal }); if (!response.ok) { throw new Error(`HTTP error! status: ${response.status}`); } const contentType = response.headers.get("content-type"); if (!contentType || !contentType.includes("application/json")) { throw new Error("Response is not JSON. The backend server may not be running or the API URL is incorrect."); } const data = await response.json(); if (!Array.isArray(data)) { throw new Error('API returned non-array data'); } return data; } finally { clearTimeout(timeoutId); } } catch (error) { lastError = error; console.error(`Attempt ${attempt + 1} failed:`, error); // Don't retry on certain errors if (error.message.includes('not set') || error.message.includes('non-array data')) { throw error; } // Wait before retrying (except on last attempt) if (attempt < maxRetries - 1) { await new Promise(resolve => setTimeout(resolve, retryDelay * (attempt + 1))); } } } throw lastError; };
🧹 Nitpick comments (1)
frontend/src/utils/api/fetchData.js (1)
39-42: Improve error messages by distinguishing timeout errors.While rethrowing errors correctly surfaces failures (per PR objectives), the catch block doesn't distinguish
AbortErrorfrom timeout vs. other errors. Users would benefit from knowing specifically when a request timed out.🔎 Proposed improvement
} catch (error) { + if (error.name === 'AbortError') { + console.error('Request timed out after 10 seconds'); + throw new Error('Request timed out. Please check your connection and try again.'); + } console.error('Error fetching data:', error); throw error; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/utils/api/fetchData.js
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/utils/api/fetchData.js (1)
frontend/src/utils/api/weatherService.js (2)
data(15-15)response(7-9)
🔇 Additional comments (2)
frontend/src/utils/api/fetchData.js (2)
34-36: LGTM: Good runtime validation.The array validation prevents downstream errors and provides a clear error message when the API returns unexpected data.
1-43: Error handling is already properly implemented—no action needed.The calling code in
App.js(lines 23-29) already has a try-catch block that properly handles the errors thrown byfetchData. When an error occurs, it is logged and the UI gracefully renders with an empty array.Regarding timeout consistency:
weatherService.jsandfetchDataserve different purposes. The 10-second timeout is appropriate for internal backend calls that should respond quickly, but external weather APIs may have different latency characteristics and should not necessarily use the same timeout strategy.
|
Fix on issue |
Fixed a bug where API errors were caught silently, causing the UI to freeze or show empty data. Added a retry mechanism, request timeouts, and better error logging.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.