-
Notifications
You must be signed in to change notification settings - Fork 3
fix(PE-8428): reduce use of dispatchArNSState #800
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: develop
Are you sure you want to change the base?
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ 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 comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
6382b85 to
3ad7972
Compare
| }) | ||
| : false); | ||
|
|
||
| console.log(domainData); |
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.
| console.log(domainData); |
| const [isLoading, setIsLoading] = useState(isLoadingDomains || isLoadingAnts); | ||
|
|
||
| useEffect(() => { | ||
| setIsLoading( | ||
| isLoadingDomains || isLoadingAnts || isLoadingAntsRequireUpdate, | ||
| ); | ||
| }, [ | ||
| isLoadingDomains, | ||
| isLoadingAnts, | ||
| isRefetchingDomains, | ||
| isRefetchingAnts, | ||
| isLoadingAntsRequireUpdate, | ||
| ]); |
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.
useMemo is more concise here. Also some unnecessary deps in here, should just need to watch the three booleans that set the variable.
| export const useAntsRequireUpdate = (): { | ||
| ants: string[]; | ||
| isLoading: boolean; | ||
| } => { | ||
| const { data: antData = {}, isLoading, isRefetching } = useAntsForWallet(); | ||
| const { | ||
| data: latestAntVersion, | ||
| isLoading: isLoadingLatestAntVersion, | ||
| isRefetching: isRefetchingLatestAntVersion, | ||
| } = useLatestANTVersion(); | ||
| return { | ||
| ants: Object.keys(antData) | ||
| .filter((processId) => { | ||
| return +antData[processId].version < +(latestAntVersion?.version || 0); | ||
| }) | ||
| .map((processId) => processId), | ||
| isLoading: | ||
| isLoading || | ||
| isLoadingLatestAntVersion || | ||
| isRefetching || | ||
| isRefetchingLatestAntVersion, | ||
| }; | ||
| }; |
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.
recommend useMemo on the ants and isLoading vars
deps for ants is the query data (antData and latestAntVersion) then deps for isLoading is all the loading variables.
This helps prevent unnecessary rerenders, especially if using this in a lot of elements (eg domains table on every single row and in ternaries for feature blocking inputs)
| }; | ||
|
|
||
| export const useAntsRequireUpdate = (): { | ||
| ants: string[]; |
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.
nit: antIds
| isRefetching: isRefetchingDomains, | ||
| refetch: refetchDomains, | ||
| } = useArNSRecordsForWallet(); | ||
| const { ants: antsRequireUpdate, isLoading: isLoadingAntsRequireUpdate } = |
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.
nit: antsRequireUpdate seems to indicate boolean but this is an array of process id's - antsRequiringUpdate / useAntsRequiringUpdate might be higher signal here.
| const { data: antVersions = {} } = useANTVersions(); | ||
| const { data: latestAntVersion } = useLatestANTVersion(); | ||
| return useQuery({ | ||
| queryKey: ['ants-with-metadata', walletAddress?.toString()], |
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.
hyperbeamUrl, aoNetwork.ANT, accessControlList, antVersions should be added to the query key so that if they are updated the data is reevaluated. Main concern there is accessControlList
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.
And actually doesn't look like we need wallet address here. Fine to have it, but it doesn't affect the query.
| const [{ walletAddress }] = useWalletState(); | ||
| const [{ arioContract }] = useGlobalState(); | ||
| return useQuery({ | ||
| queryKey: ['arns-records-for-wallet', walletAddress?.toString()], |
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.
arioProcessId, aoNetwork.ARIO, and hyperbeamUrl should be added to the query key here
| } | ||
| return arnsRecordsForWallet; | ||
| }, | ||
| staleTime: 60 * 60 * 1000, // 1 hour |
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.
enabled: !!walletAddress
| const { | ||
| items: newRecords, | ||
| hasMore: nextHasMore, | ||
| nextCursor, | ||
| } = await arioContract.getArNSRecordsForAddress({ | ||
| address: walletAddress?.toString() ?? '', | ||
| cursor, | ||
| }); |
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.
nit: explicitly throw if no wallet address to satisfy type (also see comment on enabling the query)
Also, think we need to specify limit: 1000 else it will default to 100?
|
|
||
| // TODO: move all of these async calls to unique hooks and remove the need for the dispatchArNSUpdate action | ||
| const arioContract = ARIO.init({ | ||
| hyperbeamUrl, |
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.
this was added so the ANTRegistry internally in the sdk uses the hyperbeam ACL - why does it need to be removed?
| version: number; | ||
| processMeta: TransactionEdge['node'] | null; | ||
| errors?: Error[]; | ||
| isLatestVersion?: boolean; |
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.
is the intention of nullish to indicate to not display upgrade prompts?
| await queryClient.invalidateQueries({ | ||
| queryKey: ['ant', processId], |
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.
This is a less dependant way to indicate the query key - omitting 'ant' will also refresh any apis using the process id.
Query key is position sensitive, so if processId is changed in the future to another index this breaks. Using the predicate helps future proof a bit there.
| await queryClient.invalidateQueries({ | |
| queryKey: ['ant', processId], | |
| await queryClient.invalidateQueries({ | |
| predicate: ({ queryKey }) => queryKey.includes("ant") && queryKey.includes(processId), |
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.
Another note there, check in with the new hooks on things relying on state... I believe the useAntsForWallet hook calls this in its query, but its state may not be updated in this refresh.
Thats a case where including the process id in the query key and then predicating on the process ID can help in cache control.
|
|
||
| return antsWithMetadata.reduce((acc, item) => ({ ...acc, ...item }), {}); | ||
| }, | ||
| }); |
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.
Should add a stale time, and enabled prop.
Should only be enabled if we have the accessControlList - not having version is fine.
| return { | ||
| ants: Object.keys(antData) | ||
| .filter((processId) => { | ||
| return +antData[processId].version < +(latestAntVersion?.version || 0); |
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.
need to check the role on these ants as well, if its not an Owner role, then needs to be filters out.
Or, if not, then we need to update the permissions/handling on the buttons (which tbh is probably a better idea to show the actual state with a different UI)
|
Visit the preview URL for this PR (updated for commit 7b0514f): https://arns-portal--pr800-manage-improvements-kvttgo21.web.app (expires Sun, 24 Aug 2025 11:51:15 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 1f59769442cb89cf2a92042a342e51fe94047b94 |
No description provided.