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
|
|
📝 WalkthroughWalkthroughThe changes simplify HTTP error handling in the EnsRainbow SDK by removing JSON error payload parsing in the 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.
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/ensrainbow-sdk/src/client.ts (1)
403-407:⚠️ Potential issue | 🔴 CriticalFix the type error and narrow the parse-failure fallback.
Line 405 fails typecheck (
json()resolves tounknownhere), and Line 406 currently swallows all parse/runtime errors instead of only the non-JSON case. This is a release-blocking correctness issue (CI already failing).Proposed fix
- const errorMessage = await response - .json() - .then((data: { error?: string }) => data.error) - .catch(() => undefined); + let errorMessage: string | undefined; + try { + const data = (await response.json()) as { error?: unknown }; + if (typeof data.error === "string") { + errorMessage = data.error; + } + } catch (error) { + if (!(error instanceof SyntaxError)) { + throw error; + } + } throw new Error(errorMessage ?? `Failed to fetch ENSRainbow config: ${response.statusText}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ensrainbow-sdk/src/client.ts` around lines 403 - 407, The current use of response.json() assumes a typed result and indiscriminately catches all errors; change to explicitly await response.json(), narrow-guard the parsed value (ensure it's a non-null object and has a string 'error' property) to assign errorMessage, and only swallow JSON parse errors (SyntaxError) while rethrowing other exceptions. Concretely, replace the promise chain with a try/await block that does: const parsed = await response.json(); if (parsed && typeof parsed === 'object' && 'error' in parsed && typeof (parsed as any).error === 'string') errorMessage = (parsed as any).error; catch (e) { if (!(e instanceof SyntaxError)) throw e; } then throw new Error(errorMessage ?? `Failed to fetch ENSRainbow config: ${response.statusText}`), updating the code around response, errorMessage to use these checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/ensrainbow-sdk/src/client.ts`:
- Around line 403-407: The current use of response.json() assumes a typed result
and indiscriminately catches all errors; change to explicitly await
response.json(), narrow-guard the parsed value (ensure it's a non-null object
and has a string 'error' property) to assign errorMessage, and only swallow JSON
parse errors (SyntaxError) while rethrowing other exceptions. Concretely,
replace the promise chain with a try/await block that does: const parsed = await
response.json(); if (parsed && typeof parsed === 'object' && 'error' in parsed
&& typeof (parsed as any).error === 'string') errorMessage = (parsed as
any).error; catch (e) { if (!(e instanceof SyntaxError)) throw e; } then throw
new Error(errorMessage ?? `Failed to fetch ENSRainbow config:
${response.statusText}`), updating the code around response, errorMessage to use
these checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 75a91951-6e1c-4f9d-8d78-486e184a8163
📒 Files selected for processing (2)
apps/ensindexer/package.jsonpackages/ensrainbow-sdk/src/client.ts
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 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.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/ensrainbow-sdk/src/client.test.ts`:
- Around line 320-324: Add a new unit test next to the existing non-JSON
fallback test in packages/ensrainbow-sdk/src/client.test.ts that verifies JSON
error-body precedence: mock the fetch used by the tests
(mockFetch.mockResolvedValueOnce) to return an object with ok: false and a json
method that resolves to { error: "Your error message" } (and statusText if
desired), then call client.config() and assert the promise rejects with an error
containing that JSON error string (e.g., await
expect(client.config()).rejects.toThrow(/Your error message/)); mirror the
structure and naming of the existing test so it's clear this is the
JSON-precedence companion.
In `@packages/ensrainbow-sdk/src/client.ts`:
- Around line 402-404: The current error throw uses only response.statusText and
discards any JSON error payload; update the error path that throws for non-ok
responses to read the response body first and include its details: attempt to
parse await response.json() (falling back to await response.text() if parsing
fails), extract a useful message (e.g., body.message || body.error || the raw
text or JSON stringified body), and then throw a new Error that includes
response.status, response.statusText, and the extracted error details; reference
the existing response variable and the throw that currently reads `Failed to
fetch ENSRainbow config: ${response.statusText}` and replace it with the richer
message as described.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c6357201-19b3-4a2f-9e02-44b5e92ccd29
📒 Files selected for processing (3)
apps/ensindexer/src/lib/version-info.tspackages/ensrainbow-sdk/src/client.test.tspackages/ensrainbow-sdk/src/client.ts
💤 Files with no reviewable changes (1)
- apps/ensindexer/src/lib/version-info.ts
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