fix: throw better error when ensrainbow fails to serve json#1713
fix: throw better error when ensrainbow fails to serve json#1713
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe pull request improves error handling in the EnsRainbow SDK's API client to gracefully manage non-JSON error responses by adding a fallback message, adds a test case validating this behavior, and updates the ensindexer build script to invoke TypeScript compiler through pnpm. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
Greptile SummaryThis PR fixes a crash in Confidence Score: 5/5
Last reviewed commit: 9268bbf |
There was a problem hiding this comment.
Pull request overview
Improves resilience of the ENSRainbow SDK client when /v1/config returns a non-JSON error response, preventing JSON parse failures from bubbling up unexpectedly and adding test coverage for that scenario.
Changes:
- Wrap
response.json()inEnsRainbowApiClient.config()error handling with a try/catch to fall back to astatusText-based message when the error body isn’t valid JSON. - Add a unit test covering the non-JSON error body case.
- Adjust
apps/ensindexertypecheckscript to runtscviapnpm exec.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/ensrainbow-sdk/src/client.ts | Makes config-fetch error handling robust to invalid/non-JSON error bodies. |
| packages/ensrainbow-sdk/src/client.test.ts | Adds a regression test ensuring fallback messaging on non-JSON error bodies. |
| apps/ensindexer/package.json | Updates the typecheck script invocation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| } catch { | ||
| // response body is not valid JSON |
There was a problem hiding this comment.
The catch here swallows any error thrown by response.json() (not just invalid JSON). That can hide unexpected failures like body stream errors and make debugging harder. Consider catching the error, ignoring only SyntaxError (non-JSON bodies), and rethrowing anything else.
| } catch { | |
| // response body is not valid JSON | |
| } catch (error) { | |
| // response body is not valid JSON; only ignore JSON syntax errors | |
| if (!(error instanceof SyntaxError)) { | |
| throw error; | |
| } |
| "test": "vitest", | ||
| "lint": "biome check --write .", | ||
| "lint:ci": "biome ci", | ||
| "typecheck": "tsc --noEmit" | ||
| "typecheck": "pnpm exec tsc --noEmit" |
There was a problem hiding this comment.
This script change (wrapping tsc with pnpm exec) isn't mentioned in the PR description and is unrelated to the ENSRainbow JSON error handling change. Please either update the PR description to explain why this is needed, or move this to a separate PR to keep the change set focused.
| } catch { | ||
| // response body is not valid JSON | ||
| } | ||
| throw new Error(message ?? `Failed to fetch ENSRainbow config: ${response.statusText}`); |
There was a problem hiding this comment.
The PR checklist claims this "does not change runtime behavior or semantics", but this change intentionally alters error behavior for non-OK + non-JSON responses (previously a SyntaxError could propagate; now it becomes a generic Error with a fallback message). Please update the PR description/checklist to reflect that this is a behavioral change in error handling.
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
| json: () => Promise.reject(new SyntaxError("Unexpected token")), | ||
| }); | ||
|
|
||
| await expect(client.config()).rejects.toThrow("Failed to fetch ENSRainbow config: Not Found"); |
There was a problem hiding this comment.
Line 353 in client.test.ts exceeds the 100 character line width limit configured in biome.jsonc
| } catch { | ||
| // response body is not valid JSON |
There was a problem hiding this comment.
| } catch { | |
| // response body is not valid JSON | |
| } catch (error) { | |
| // Only ignore SyntaxError from non-JSON bodies | |
| if (!(error instanceof SyntaxError)) { | |
| throw error; | |
| } |
Bare catch block masks unexpected errors from response.json() beyond SyntaxError, hiding stream failures and other runtime issues
Summary
response.json()in the error path ofEnsRainbowApiClient.config()with try-catch so non-JSON error responses (e.g. plain text404 Not Found) fall through to the statusText-based fallback message instead of throwing aSyntaxErrorWhy
404 Not Found),response.json()throws aSyntaxErrorthat propagates unhandled through the ENSIndexer's/api/configendpoint, causing theEnsDbWriterWorkerto crashand take down the entire process
Testing
pnpm testinpackages/ensrainbow-sdk, 25 tests)pnpm devagainst a local ENSRainbow that returns404 Not Foundfor/v1/configChecklist