fix: absorb fetch-level failures in apiFetch instead of throwing - #49
Conversation
apiFetch called fetch() with no try/catch, so a transport-level failure (offline, DNS, aborted request) propagated as an unhandled rejection through every chartApi.js wrapper, since they only guard `if (!res.ok)`. Worst at Promise.all call sites (SyncToModal, SyncFromModal) where one rejection skips sibling .catch entirely. Fixed centrally in apiFetch itself so every existing wrapper's `if (!res.ok) return <fallback>` transparently absorbs transport failures too, without touching any of chartApi.js's ~25 wrappers. Fixes #45
|
Preview environment failed to start. |
📝 WalkthroughWalkthroughapiFetch is changed from a synchronous fetch wrapper into an async function that catches fetch rejections and returns a fallback response object (ok: false, status: 0, json() returning the error). New unit tests validate this fallback for apiFetch and chartApi's listCharts, getSyncRegistry, and createSync. ChangesapiFetch Network Failure Handling
Estimated code review effort: 1 (Trivial) | ~5 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant apiFetch
participant fetch
Caller->>apiFetch: apiFetch(path, opts)
apiFetch->>fetch: fetch(url, opts)
fetch-->>apiFetch: reject(Error)
apiFetch-->>Caller: { ok: false, status: 0, json() }
Related issues: Suggested labels: bug, tests Suggested reviewers: null-ptr-exception Poem: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/chartApi.test.js (1)
1-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood coverage for the three wrappers; consider extending to remaining
chartApi.jsexports.Tests validate
listCharts,getSyncRegistry, andcreateSyncfallback correctly on network failure, matching the documented upstream contract. Since issue#45calls out a fix across the entirechartApi.jsfile (including endpoints used bySyncToModal/SyncFromModal), consider adding similar fallback tests for any remaining wrappers not covered here to fully close out the issue.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/chartApi.test.js` around lines 1 - 28, The current tests cover the main fallback wrappers in chartApi.js, but the issue is broader and should be validated across all exported request helpers. Add similar network-failure fallback assertions for any remaining chartApi.js functions referenced by SyncToModal and SyncFromModal, using the existing patterns in listCharts, getSyncRegistry, and createSync so each wrapper resolves to its documented safe default instead of throwing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/unit/chartApi.test.js`:
- Around line 1-28: The current tests cover the main fallback wrappers in
chartApi.js, but the issue is broader and should be validated across all
exported request helpers. Add similar network-failure fallback assertions for
any remaining chartApi.js functions referenced by SyncToModal and SyncFromModal,
using the existing patterns in listCharts, getSyncRegistry, and createSync so
each wrapper resolves to its documented safe default instead of throwing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 58d73374-1041-42d0-954e-57ea872d272f
📒 Files selected for processing (3)
src/lib/apiFetch.jstests/unit/apiFetch.test.jstests/unit/chartApi.test.js
rophy
left a comment
rophy
left a comment
There was a problem hiding this comment.
Good fix — centralizing transport-error handling in apiFetch lets all existing if (!res.ok) guards absorb network failures transparently. Clean and well-tested.
2 participants
Summary
src/utils/chartApi.jsonly guardsif (!res.ok).apiFetchcalledfetch()with no try/catch, so a transport-level failure (offline, DNS, aborted request) propagated as an unhandled rejection through every wrapper.Promise.allcall sites (SyncToModal,SyncFromModal), where a single rejected promise skips sibling.catches entirely and surfaces as an uncaught error in the modal.apiFetchitself (per the issue's own suggested approach) rather than touching all ~25 wrappers individually — avoids two inconsistent error-handling styles in the same file. Each wrapper's existingif (!res.ok) return <fallback>now transparently absorbs transport failures too.Test plan
npm test— 337/338 passing (1 pre-existing unrelated failure:git-lib.test.jslocale mismatch on system git output, not touched by this change)npm run lint— cleanapiFetchresolves to a failed response instead of throwing whenfetchrejectstests/unit/chartApi.test.js:listCharts,getSyncRegistry,createSyncall return their documented fallback shape instead of throwing when the network request failsfetchmock thatapiFetchno longer throwsCloses #45
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests