fix(search): fix debounce on search input, remove use of global arns domains #855
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughDomainSearch now uses useRegistrationStatus with a 500ms debounce, exposes Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant HomeSearch
participant DomainSearch
participant useRegistrationStatus
User->>HomeSearch: Type domain
HomeSearch->>DomainSearch: pass props (including setIsReturnedName)
DomainSearch->>DomainSearch: update searchQuery (immediate)
Note over DomainSearch: debounce 500ms before querying
DomainSearch->>useRegistrationStatus: query(debouncedSearchQuery)
useRegistrationStatus-->>DomainSearch: { record, returnedName, isAvailable, isReturnedName, isLoading }
DomainSearch->>HomeSearch: setIsReturnedName(isReturnedName)
DomainSearch-->>HomeSearch: isSearching / availability / validation
HomeSearch-->>User: render UI (Searching / RECENTLY RETURNED / AVAILABLE / NOT AVAILABLE)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Visit the preview URL for this PR (updated for commit 8b31b9a): https://arns-portal--pr855-pe-8449-debounce-inp-pzhhym13.web.app (expires Wed, 27 Aug 2025 18:21:37 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 1f59769442cb89cf2a92042a342e51fe94047b94 |
| }) { | ||
| const [searchParams, setSearchParams] = useSearchParams(); | ||
| const location = useLocation(); | ||
| const { data: arnsDomains, isLoading: loadingArnsRegistryDomains } = |
There was a problem hiding this comment.
we will continue to remove the use of this hook throughout the app
There was a problem hiding this comment.
This is only used in one other place - add undername modal. I suggest we update there and delete the hook.
There was a problem hiding this comment.
will do that in a separate PR
dtfiedler
force-pushed
the
PE-8449-debounce-input
branch
from
9956fec to
39b30e4
Compare
There was a problem hiding this comment.
this component now manages the debounce, as it contains the actual input HTML component
dtfiedler
changed the title
dtfiedler
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/components/inputs/DomainSearch.tsx (1)
58-58: Consider using a more specific type for the timeout refThe
NodeJS.Timeouttype is Node.js specific. In a browser environment, use the browser's native timeout type for better type safety.- const debounceTimeoutRef = useRef<NodeJS.Timeout>(); + const debounceTimeoutRef = useRef<ReturnType<typeof setTimeout>>();
🧹 Nitpick comments (1)
src/components/inputs/Search/HomeSearch.tsx (1)
140-140: Consider simplifying the complex conditional logicThe nested ternary operators in this line make the code hard to read and maintain. Consider extracting this into a helper function or using early returns.
- ) : isSearching || domainQuery.length == 0 || !isAvailable ? ( + ) : shouldShowSpacerHeader() ? (Add a helper function before the return statement:
const shouldShowSpacerHeader = () => { return isSearching || domainQuery.length === 0 || !isAvailable; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/inputs/DomainSearch.tsx(7 hunks)src/components/inputs/Search/HomeSearch.tsx(6 hunks)src/hooks/useRegistrationStatus/useRegistrationStatus.tsx(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/components/inputs/DomainSearch.tsx (1)
src/hooks/useRegistrationStatus/useRegistrationStatus.tsx (1)
useRegistrationStatus(6-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build_and_test / lint_test_build
- GitHub Check: lint_test_build
🔇 Additional comments (7)
src/hooks/useRegistrationStatus/useRegistrationStatus.tsx (1)
1-25: LGTM! Clean implementation of the registration status hook.The hook provides a clear API with proper loading state management and correctly derives
isReturnedNamefrom thereturnedNamedata.src/components/inputs/Search/HomeSearch.tsx (2)
39-39: Good simplification by removing debounce from HomeSearchMoving the debounce logic to DomainSearch centralizes the input handling and removes redundant code. The immediate state updates in HomeSearch with validation happening on every change is appropriate.
208-259: UI logic correctly implements returned names featureThe conditional rendering properly handles all states: searching, invalid domain, returned name, available, and not available. The tooltip for "RECENTLY RETURNED" provides good user context.
src/components/inputs/DomainSearch.tsx (4)
70-96: Well-structured availability handler with proper state managementThe function correctly handles empty search queries, loading states, and availability results. The early returns improve readability and the state updates are logically grouped.
106-126: Good implementation of debounced search with immediate feedbackThe debounce implementation correctly:
- Provides immediate feedback for empty inputs
- Clears existing timeouts before setting new ones
- Uses a reasonable 300ms delay for user input
167-174: Good cleanup implementation for the debounce timeoutThe effect properly cleans up the timeout on component unmount, preventing memory leaks.
136-139: No action needed—dependency onisAvailableis correct and won’t cause an infinite loopThe
isAvailablein the dependency array comes from theuseRegistrationStatushook, not from a local state setter, so callingsetIsAvailable(isAvailable)insideavailabilityHandleronly updates parent state (and bails out if the value hasn’t changed). This won’t retrigger the hook or cause an infinite loop. The effect correctly listens for changes to the hook’s loading and availability values to keep the UI in sync.Likely an incorrect or invalid review comment.
dtfiedler
force-pushed
the
PE-8449-debounce-input
branch
from
1121644 to
c39542c
Compare
…eries, update debounce logic
dtfiedler
force-pushed
the
PE-8449-debounce-input
branch
from
c39542c to
e5b61ac
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
src/components/pages/Register/Register.tsx (1)
160-165: Refetch result not used;antModuleIdremains stale within the same tick.
await refetchAntVersion()updates data asynchronously, butantModuleIdhere references memoized state from the previous render. Use the refetch result to determine the module ID synchronously and prefer it when building the payload.Apply:
- if (!antModuleId) { - await refetchAntVersion(); - if (!antModuleId) { - throw new Error('No ANT Module available, try again later'); - } - } + let effectiveAntModuleId = antModuleId; + if (!effectiveAntModuleId) { + const res = await refetchAntVersion(); + effectiveAntModuleId = res.data?.moduleId; + if (!effectiveAntModuleId) { + throw new Error('No ANT Module available, try again later'); + } + }And pass
effectiveAntModuleIdlater whereantModuleIdis used for the payload.
♻️ Duplicate comments (1)
src/components/inputs/DomainSearch.tsx (1)
57-57: Use a browser-safe timeout type.
NodeJS.Timeoutis incorrect in the browser; useReturnType<typeof setTimeout>instead to satisfy both Node and DOM typings.- const debounceTimeoutRef = useRef<NodeJS.Timeout>(); + const debounceTimeoutRef = useRef<ReturnType<typeof setTimeout>>();
🧹 Nitpick comments (5)
src/hooks/useArNSRecord.tsx (1)
11-13: Validation check is redundant; simplify for clarity.
isARNSDomainNameValid({ name })already returnsfalseforundefinedor empty strings, so the explicitname === undefinedclause is unnecessary.- if (!isARNSDomainNameValid({ name }) || name === undefined) + if (!isARNSDomainNameValid({ name })) throw new Error('Invalid ArNS name');src/hooks/useArNSDomainPriceList.tsx (1)
81-82: Consider moving all gating intoenabledfor consistency.You already gate by
isAvailable && domain.length > 0here. The additional checks insidequeryFnduplicate this logic. To reduce branching and simplify testing, rely solely onenabledand drop the innerif.src/components/inputs/DomainSearch.tsx (3)
90-96: Differentiate error messages for returned names vs registered names.You already surface
isReturnedNameto consumers; set a clearer message to reduce confusion for recently returned domains.- setValidationError(isAvailable ? '' : 'This domain is already taken'); + setValidationError( + isAvailable + ? '' + : isReturnedName + ? 'This domain was recently returned and will be available soon.' + : 'This domain is already taken' + );
122-126: Remove debugconsole.logor guard behind a dev flag.Leaving logs in the UI spams the console and can affect performance in tight loops.
- console.log('setting debounced search query to', newSearchQuery); setDebouncedSearchQuery(newSearchQuery);
71-96: ConfirmsetIsValidDomainusage or remove the prop.This component no longer updates
setIsValidDomainexcept inreset(). If callers still rely on it, consider setting it inavailabilityHandlerbased on input validity; otherwise, remove to reduce API surface.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/components/inputs/DomainSearch.tsx(4 hunks)src/components/pages/Register/Register.tsx(3 hunks)src/hooks/useArNSDomainPriceList.tsx(3 hunks)src/hooks/useArNSRecord.tsx(1 hunks)src/hooks/useRegistrationStatus/useRegistrationStatus.tsx(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
src/hooks/useArNSRecord.tsx (2)
src/state/contexts/GlobalState.tsx (1)
useGlobalState(135-136)src/utils/searchUtils/searchUtils.ts (1)
isARNSDomainNameValid(17-28)
src/hooks/useArNSDomainPriceList.tsx (1)
src/hooks/useRegistrationStatus/useRegistrationStatus.tsx (1)
useRegistrationStatus(15-69)
src/hooks/useRegistrationStatus/useRegistrationStatus.tsx (1)
src/state/contexts/GlobalState.tsx (1)
useGlobalState(135-136)
src/components/inputs/DomainSearch.tsx (3)
src/hooks/useRegistrationStatus/useRegistrationStatus.tsx (1)
useRegistrationStatus(15-69)src/utils/searchUtils/searchUtils.ts (1)
lowerCaseDomain(132-134)src/utils/constants.ts (1)
MAX_ARNS_NAME_LENGTH(55-55)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build_and_test / lint_test_build
- GitHub Check: lint_test_build
🔇 Additional comments (2)
src/components/pages/Register/Register.tsx (2)
85-87: LGTM: Hook rename alignment.Good migration to
isLoadingfromuseRegistrationStatus. Downstream usage viaisValidatingRegistrationis clear.
102-109: Behavior change: automatic advance now occurs whenevernameparam exists. Verify UX.Previously this effect gated on a
redirectparam; now it always callshandleNext()ifnameexists and the balance is ready. This can auto-advance users unexpectedly when navigating to/register/:name.
- If intentional, consider adding a comment and possibly a small delay to avoid surprising transitions.
- If unintentional, restore a guard (e.g., query param or state flag) to explicitly opt-in.
I can provide a targeted patch either way—confirm desired behavior.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/hooks/useArNSRecord.tsx (2)
10-12: Remove redundant undefined check; rely onenabledvalidation, and keep the queryFn non-throwing (optional).
isARNSDomainNameValid({ name })already returns false for undefined/empty; the explicitname === undefinedis redundant.- Since
enabledgates execution when invalid, throwing here is unnecessary noise. If you want a defensive guard, keep it without the redundant|| name === undefined.Suggested minimal change:
- if (!isARNSDomainNameValid({ name }) || name === undefined) - throw new Error('Invalid ArNS name'); + if (!isARNSDomainNameValid({ name })) { + throw new Error('Invalid ArNS name'); + }Alternatively (optional), drop the throw entirely to keep this queryFn value-oriented and non-throwing.
17-18: Disable retries to avoid noisy refetching for predictable non-error states.Per earlier discussion, add
retry: falseso invalid-name or “not found” conditions don’t trigger retries.enabled: isARNSDomainNameValid({ name }), + retry: false, staleTime: 4 * 60 * 60 * 1000, // 4 hours
🧹 Nitpick comments (3)
src/hooks/useRegistrationStatus/useRegistrationStatus.tsx (3)
8-15: Doc wording nits.Tighten language and use “different from” (not “different than”); clarify gating/ordering.
-/** - * This hook determines if a domain is available, returned, or reserved. It uses queries that are - * different than the ones used in the useArNSRecord and useReturnedName hooks as we want to call them - * in specific order and we want to avoid calling the returned name query if the name is registered. - * - * @param domain - The domain to check. - * @returns An object with the registration status of the domain (e.g. isAvailable, isReturnedName, isReserved, isLoading) - */ +/** + * Determines if a domain is available, returned, or reserved. + * Uses a specific query ordering (different from useArNSRecord/useReturnedName) to avoid + * calling the returned-name query when the name is already registered. + * + * @param domain - The domain to check. + * @returns Registration status: record, returnedName, isAvailable, isReturnedName, isReserved, isLoading. + */
20-21: Comment is inaccurate with currentenabledgating.This query does not “always run if the name is not reserved”; it also requires a valid domain per
isARNSDomainNameValid.- // this query always runs if the name is not reserved + // this query runs when the name is not reserved and passes validation
23-33: Remove unreachable early-return for empty domain; validation already gates execution.
enabledprevents execution for empty/invalid domains, so thedomain.length === 0branch is dead. Simplify the queryFn.- queryFn: () => { - if (domain.length === 0) { - return null; - } - - // getArNSRecord returns undefined if the name is not registered, so convert that to null for caching purposes - return arioContract - .getArNSRecord({ name: domain }) - .then((r) => (r === undefined ? null : r)); // null is serializable, undefined is not - }, + queryFn: () => + // getArNSRecord returns undefined if the name is not registered, so convert that to null for caching purposes + arioContract + .getArNSRecord({ name: domain }) + .then((r) => (r === undefined ? null : r)), // null is serializable, undefined is not
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/hooks/useArNSRecord.tsx(1 hunks)src/hooks/useRegistrationStatus/useRegistrationStatus.tsx(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/hooks/useRegistrationStatus/useRegistrationStatus.tsx (2)
src/state/contexts/GlobalState.tsx (1)
useGlobalState(135-136)src/utils/searchUtils/searchUtils.ts (1)
isARNSDomainNameValid(17-28)
src/hooks/useArNSRecord.tsx (2)
src/state/contexts/GlobalState.tsx (1)
useGlobalState(135-136)src/utils/searchUtils/searchUtils.ts (1)
isARNSDomainNameValid(17-28)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build_and_test / lint_test_build
- GitHub Check: lint_test_build
🔇 Additional comments (4)
src/hooks/useArNSRecord.tsx (2)
14-16: Ensure thenameargument is non-null for type-safety when callinggetArNSRecord.If
getArNSRecordexpects a string (most likely), add a non-null assertion after your guard. This also aligns with the earlier guard andenabledvalidation.- const record = await arioContract.getArNSRecord({ name }); + const record = await arioContract.getArNSRecord({ name: name! }); return record ?? null; // null is serializable, undefined is notIf the contract method already accepts
string | undefined, this change is optional. Otherwise, it's necessary to satisfy TS.
14-16: Returningnullfor “not found” is the right call.Good normalization for serializability and clearer consumer logic.
src/hooks/useRegistrationStatus/useRegistrationStatus.tsx (2)
46-53: Good normalization of returned-name “not found” to null.Catching and mapping to
nullkeeps the hook value-oriented and serializable; converting anyundefinedtonullmaintains consistency with the record query.
33-35: Gating and retry settings look solid.Valid-domain gating and
retry: falsealign with the debounce/search UX and prevent noisy retries during typing.
| // NOTE: this is a hard coded list, if the reserved names are ever updated (requiring a multi-sig vote) we'll need to update this | ||
| const RESERVED_NAMES = ['www']; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid duplicating the reserved-names source of truth. Centralize it.
You’re hardcoding ['www'] here while isARNSDomainNameValid also encodes www as invalid. This duplication is brittle. Prefer a single shared constant/utility (e.g., RESERVED_ARNS_NAMES or isReservedArnsName(name)) in @src/utils and consume it here and in validators.
🤖 Prompt for AI Agents
In src/hooks/useRegistrationStatus/useRegistrationStatus.tsx around lines 5-7
you’re hardcoding RESERVED_NAMES = ['www'] which duplicates the source of truth
in the validator; remove this local constant and instead import a shared
reserved-names constant or predicate from @src/utils (e.g., RESERVED_ARNS_NAMES
or isReservedArnsName) and use that here; also add/update the shared export in
@src/utils (and update the validator to consume that same export if not already)
so both places reference the single centralized value/function.
| enabled: | ||
| !isReserved && | ||
| !recordQuery.data && | ||
| !recordQuery.isLoading && | ||
| isARNSDomainNameValid({ name: domain }), | ||
| retry: false, | ||
| staleTime: 4 * 60 * 60 * 1000, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prevent running returned-name query on record-query errors; gate on success and null.
If recordQuery errors (e.g., transient network/contract error), this will still run the returned-name query because data is undefined and isLoading is false. Gate on success and explicit null to ensure we only check returned-name when we know the name is unregistered.
- enabled:
- !isReserved &&
- !recordQuery.data &&
- !recordQuery.isLoading &&
- isARNSDomainNameValid({ name: domain }),
+ enabled:
+ !isReserved &&
+ recordQuery.isSuccess &&
+ recordQuery.data === null &&
+ isARNSDomainNameValid({ name: domain }),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| enabled: | |
| !isReserved && | |
| !recordQuery.data && | |
| !recordQuery.isLoading && | |
| isARNSDomainNameValid({ name: domain }), | |
| retry: false, | |
| staleTime: 4 * 60 * 60 * 1000, | |
| }); | |
| enabled: | |
| !isReserved && | |
| recordQuery.isSuccess && | |
| recordQuery.data === null && | |
| isARNSDomainNameValid({ name: domain }), | |
| retry: false, | |
| staleTime: 4 * 60 * 60 * 1000, | |
| }); |
🤖 Prompt for AI Agents
In src/hooks/useRegistrationStatus/useRegistrationStatus.tsx around lines 54 to
61, the enabled condition for the returned-name query currently treats
recordQuery.data === undefined (e.g., on error) the same as null and will run
the query; change the gate to require recordQuery.isSuccess and recordQuery.data
=== null (instead of !recordQuery.data and !recordQuery.isLoading) so the
returned-name query only runs when the record query completed successfully and
explicitly returned null; keep the other checks (isReserved and
isARNSDomainNameValid) and leave retry/staleTime as-is.
There was a problem hiding this comment.
🧩 Analysis chain
Let me check the current state of the file to verify the fix has been applied.
🏁 Script executed:
#!/bin/bash
# Check the current enabled condition for returnedNameQuery in the useRegistrationStatus hook
cat src/hooks/useRegistrationStatus/useRegistrationStatus.tsx | grep -A 6 -B 2 "enabled:"
Length of output: 672
@dtfiedler I can see that the enabled condition for the returnedNameQuery still contains the original logic:
enabled:
!isReserved &&
!recordQuery.data &&
!recordQuery.isLoading &&
isARNSDomainNameValid({ name: domain }),The issue I flagged was that !recordQuery.data will be true both when the query returns null (name not registered) and when it fails with an error (where data is undefined). This could cause the returned-name query to run unnecessarily on record query errors.
The suggested fix was to use:
enabled:
!isReserved &&
recordQuery.isSuccess &&
recordQuery.data === null &&
isARNSDomainNameValid({ name: domain }),This ensures the returned-name query only runs when we've successfully confirmed the name is not registered, rather than also running on errors.
ʕ•ᴥ•ʔ
🧬 Code Graph Analysis Results
- File: src/state/contexts/GlobalState.tsx (lines 135-136)
export const useGlobalState = (): [GlobalState, Dispatch<GlobalAction>] =>
useContext(GlobalStateContext);
- File: src/utils/searchUtils/searchUtils.ts (lines 17-28)
export function isARNSDomainNameValid({ name }: { name?: string }): boolean {
if (
!name ||
!ARNS_NAME_REGEX.test(
emojiRegex().test(name) ? encodeDomainToASCII(name) : name,
) ||
name === 'www'
) {
return false;
}
return true;
}
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
| const { isLoading: isValidatingRegistration } = useRegistrationStatus( | ||
| name ?? domain, | ||
| ); |
There was a problem hiding this comment.
do we even need this call?
There was a problem hiding this comment.
It's used below - so i left it
| ?.getPriceForArNSIntent({ | ||
| const [leasePrice, buyPrice, turboFiatLease, turboFiatBuy] = | ||
| await Promise.all([ | ||
| arioContract.getCostDetails({ |
There was a problem hiding this comment.
These should be using queryClient.fetchQuery.... but not a big deal, the better solution here is to use the demand factor costs for the name.
Maybe add a TODO to use that from the SDK once we have it.
| returnedName === undefined && record === undefined && !isReserved; | ||
| !(recordQuery.isLoading || returnedNameQuery.isLoading) && | ||
| !record && | ||
| !returnedName && |
There was a problem hiding this comment.
if its returned, its available
Summary by CodeRabbit
New Features
UI/UX
Performance
Bug Fixes