Conversation
Includes 1) connecting with invalid URL; 2) connecting to unavailable host; 3) fetching invalid data from host.
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
There was a problem hiding this comment.
Pull request overview
This PR refines ENSAdmin’s UI handling for ENSNode connection failures by centralizing the error display and ensuring the page header still renders when the selected connection is invalid.
Changes:
- Introduces a shared
ENSNodeConnectionErrorcomponent for connection-related error UI. - Updates
SelectedENSNodeProviderto always render a providedheader, even when the selected connection URL is invalid. - Switches
RequireActiveConnectionto use the shared connection error UI on ENSNode config errors.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| apps/ensadmin/src/components/providers/selected-ensnode-provider.tsx | Adds a header prop and renders it in both valid/invalid connection flows; uses shared connection error UI for invalid URL case. |
| apps/ensadmin/src/components/layout-wrapper.tsx | Passes the existing header markup into SelectedENSNodeProvider via the new header prop. |
| apps/ensadmin/src/components/connections/require-active-connection.tsx | Uses ENSNodeConnectionError instead of ErrorInfo when useENSNodeConfig() reports an error. |
| apps/ensadmin/src/components/connections/connection-error.tsx | Adds ENSNodeConnectionError wrapper around ErrorInfo with standardized messaging. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <ENSNodeConnectionError | ||
| error={new Error(`Unable to parse ENSNode Config: ${error?.message}`)} | ||
| /> |
There was a problem hiding this comment.
ENSNodeConnectionError always renders the title "Unable to connect to ENSNode", but here the error being surfaced is "Unable to parse ENSNode Config". This makes the UI message inconsistent; either keep using ErrorInfo for config-parse failures or make ENSNodeConnectionError accept an overridable title/description.
| * @param children - React children to render within the provider context | ||
| */ | ||
| export function SelectedENSNodeProvider({ children }: PropsWithChildren) { | ||
| export function SelectedENSNodeProvider({ | ||
| header, | ||
| children, | ||
| }: PropsWithChildren<{ header: React.ReactNode }>) { |
There was a problem hiding this comment.
The JSDoc for SelectedENSNodeProvider only documents children, but the component now requires a header prop as well. Update the doc comment to describe the new prop and its purpose (rendered even when the selected connection is invalid).
| // between the selected connection being in a valid format or not. | ||
| // This logic will throw and an error and break if the selected connection | ||
| // is in an invalid format. | ||
|
|
||
| return ( |
There was a problem hiding this comment.
The TODO comment says the invalid-format path "will throw and break", but this branch now handles invalid URLs by rendering an error component. Update or remove the comment to reflect the current behavior (and clarify what still needs refactoring, if anything).
| export function ENSNodeConnectionError({ error }: { error: Error }) { | ||
| return ( | ||
| <ErrorInfo | ||
| title="Unable to connect to ENSNode" | ||
| description={`Please check your connection settings and try again. Error: ${error.message}`} |
There was a problem hiding this comment.
ENSNodeConnectionError hard-codes the title and formats the full error message into the description, which makes it awkward to reuse for non-connection failures (e.g. config parse errors). Consider accepting optional title/description overrides (or an ErrorInfoProps passthrough) so callers can keep messaging accurate while still sharing the layout.
| export function ENSNodeConnectionError({ error }: { error: Error }) { | |
| return ( | |
| <ErrorInfo | |
| title="Unable to connect to ENSNode" | |
| description={`Please check your connection settings and try again. Error: ${error.message}`} | |
| export function ENSNodeConnectionError({ | |
| error, | |
| title, | |
| description, | |
| }: { | |
| error: Error; | |
| title?: string; | |
| description?: string; | |
| }) { | |
| return ( | |
| <ErrorInfo | |
| title={title ?? "Unable to connect to ENSNode"} | |
| description={ | |
| description ?? | |
| `Please check your connection settings and try again. Error: ${error.message}` | |
| } |
| import { useENSNodeConfig } from "@ensnode/ensnode-react"; | ||
|
|
||
| import { ENSNodeConnectionError } from "@/components/connections/connection-error"; | ||
| import { ErrorInfo } from "@/components/error-info"; |
There was a problem hiding this comment.
ErrorInfo is imported but no longer used after switching to ENSNodeConnectionError; remove the unused import to keep lint/typecheck clean.
| import { ErrorInfo } from "@/components/error-info"; |
| <ENSNodeConnectionError | ||
| error={new Error(`Unable to parse ENSNode Config: ${error?.message}`)} | ||
| /> |
There was a problem hiding this comment.
Wrapping error in new Error(...) drops the original error object (stack/cause). Consider either passing the original error through to ENSNodeConnectionError (and letting it format unknowns safely), or construct the new error with { cause: error } so debugging info is preserved.
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
| <section className="p-6"> | ||
| <ErrorInfo title="Unable to parse ENSNode Config" description={error.message} /> | ||
| <ENSNodeConnectionError | ||
| error={new Error(`Unable to parse ENSNode Config: ${error?.message}`)} |
There was a problem hiding this comment.
| error={new Error(`Unable to parse ENSNode Config: ${error?.message}`)} | |
| error={new Error(`Unable to parse ENSNode Config: ${error.message}`)} |
Using optional chaining on error field when it's guaranteed to be defined by React Query's type system after status check
1 participant
Lite PR
Tip: Review docs on the ENSNode PR process
Summary
Includes:
Why
Testing
Notes for Reviewer (Optional)
Pre-Review Checklist (Blocking)