-
Notifications
You must be signed in to change notification settings - Fork 17
fix(apps/ensadmin): validate name query params on name details page #1834
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 3 commits
4bf234f
f7893a2
569f803
255190e
0a17eea
0ab0331
14e162f
72e310e
1f29be9
a205294
1fd70f1
2ff21a5
3c05ce3
bb498a4
077628b
3308ed9
e92eb03
310919a
f2c0398
32ac25e
8663711
456c3e5
298885e
d1cd779
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "ensadmin": patch | ||
| --- | ||
|
|
||
| Validate name query param as InterpretedName and NormalizedName before rendering the name details page, displaying appropriate error states for invalid or unsupported names. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,7 +3,7 @@ | |
| import { getEnsManagerNameDetailsUrl } from "@namehash/namehash-ui"; | ||
| import { useSearchParams } from "next/navigation"; | ||
|
|
||
| import type { Name } from "@ensnode/ensnode-sdk"; | ||
| import { isNormalizedName, type Name } from "@ensnode/ensnode-sdk"; | ||
|
|
||
| import { ExternalLinkWithIcon } from "@/components/link"; | ||
| import { Button } from "@/components/ui/button"; | ||
|
|
@@ -13,11 +13,14 @@ export default function ActionsNamePage() { | |
| const searchParams = useSearchParams(); | ||
| const nameParam = searchParams.get("name"); | ||
|
|
||
| const name = nameParam ? (decodeURIComponent(nameParam) as Name) : null; | ||
| const name = nameParam ? (nameParam as Name) : null; | ||
|
|
||
| const { data: namespace } = useNamespace(); | ||
|
|
||
| const ensAppProfileUrl = name && namespace ? getEnsManagerNameDetailsUrl(name, namespace) : null; | ||
| const ensAppProfileUrl = | ||
| name && isNormalizedName(name) && namespace | ||
| ? getEnsManagerNameDetailsUrl(name, namespace) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any special reason not to move the Please ignore if not relevant.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand why you mention how If we say that we don't want to link to the ENS Manager Name Details page for unnormalized names, then that decision should move into Otherwise we have to keep endlessly talking about the implementation details of unnormalized name handling which I really hate as I've been doing that over and over again for 4 years. |
||
| : null; | ||
|
|
||
| if (!ensAppProfileUrl) return null; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,7 @@ | ||
| "use client"; | ||
|
|
||
| import { ASSUME_IMMUTABLE_QUERY, useRecords } from "@ensnode/ensnode-react"; | ||
| import { type Name, type ResolverRecordsSelection } from "@ensnode/ensnode-sdk"; | ||
| import { type NormalizedName, type ResolverRecordsSelection } from "@ensnode/ensnode-sdk"; | ||
|
|
||
| import { Card, CardContent } from "@/components/ui/card"; | ||
| import { useActiveNamespace } from "@/hooks/active/use-active-namespace"; | ||
|
|
@@ -39,7 +39,7 @@ const AllRequestedTextRecords = [ | |
| ]; | ||
|
|
||
| interface NameDetailPageContentProps { | ||
| name: Name; | ||
| name: NormalizedName; | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The original issue stated:
and most recently:
|
||
| } | ||
|
|
||
| export function NameDetailPageContent({ name }: NameDetailPageContentProps) { | ||
|
|
||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm aware this is a new file, and not sure where this lives long term. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| import type { Name } from "@ensnode/ensnode-sdk"; | ||
|
|
||
| import { ErrorInfo } from "@/components/error-info"; | ||
|
|
||
| interface NameErrorProps { | ||
| name: Name; | ||
| } | ||
|
|
||
| export function InvalidNameError({ name }: NameErrorProps) { | ||
| return ( | ||
| <section className="p-6"> | ||
| <ErrorInfo | ||
| title="Invalid Name" | ||
| description={`The provided name "${name}" is not ENS normalized.`} | ||
| /> | ||
| </section> | ||
| ); | ||
| } | ||
|
|
||
| export function EncodedLabelhashUnsupportedError({ name }: NameErrorProps) { | ||
| return ( | ||
| <section className="p-6"> | ||
| <ErrorInfo | ||
| title="Encoded Labelhash Detected" | ||
| description={`The provided name "${name}" contains encoded labelhashes. Support for resolving names with encoded labelhashes is in progress and coming soon.`} | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should never take raw name values and put them in UIs. All names appearing in UIs should go through the logic we define in the |
||
| /> | ||
| </section> | ||
| ); | ||
| } | ||
|
Comment on lines
+11
to
+20
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial Consider displaying the interpreted name in the error message. Since 💡 Optional enhancement+import type { InterpretedName } from "@ensnode/ensnode-sdk";
+import { formatInterpretedNameForDisplay } from "@/lib/format-interpreted-name-for-display";
import { ErrorInfo } from "@/components/error-info";
-export function InterpretedNameUnsupportedError() {
+export function InterpretedNameUnsupportedError({ name }: { name: InterpretedName }) {
return (
<section className="p-6">
<ErrorInfo
title="Encoded Labelhash Detected"
- description="The provided name contains encoded labelhashes. Support for resolving names with encoded labelhashes is in progress and coming soon."
+ description={`The name "${formatInterpretedNameForDisplay(name)}" contains encoded labelhashes. Support for resolving names with encoded labelhashes is in progress and coming soon.`}
/>
</section>
);
}🤖 Prompt for AI Agents |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,14 +2,14 @@ | |
|
|
||
| import { EnsAvatar, NameDisplay } from "@namehash/namehash-ui"; | ||
|
|
||
| import type { ENSNamespaceId, Name } from "@ensnode/ensnode-sdk"; | ||
| import type { ENSNamespaceId, NormalizedName } from "@ensnode/ensnode-sdk"; | ||
|
|
||
| import { ExternalLinkWithIcon } from "@/components/link"; | ||
| import { Card, CardContent } from "@/components/ui/card"; | ||
| import { beautifyUrl } from "@/lib/beautify-url"; | ||
|
|
||
| interface ProfileHeaderProps { | ||
| name: Name; | ||
| name: NormalizedName; | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was changed because of the comments made about the downstream handling, and:
|
||
| namespaceId: ENSNamespaceId; | ||
| headerImage?: string | null; | ||
| websiteUrl?: string | null; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's please move the typing for things to where they originate. Ex: If this is a
Nameand not an arbitrary string, then we should apply that typing here.Please apply this idea everywhere it is relevant.
Note how the reinterpretation of this value as a
Nameis happening several lines below. I don't like that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, I've changed this to
const nameFromQuery = searchParams.get("name") as Name | nullwhich is really the only other option here as far as I can tell.