fix: domain navigation #1001
fix: domain navigation #1001
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@blessingbytes is attempting to deploy a commit to the LFG Labs Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe pull request modifies the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
pages/identities/[tokenId].tsx (3)
208-220: Add accessibility attributes to navigation buttons.The identity navigation buttons should include proper ARIA labels for better accessibility.
<button + aria-label={`Navigate to identity ${domain.id}`} className={`${ domain.id === router.query.tokenId ? "text-[#402D28] hover:text-[#CDCCCC]" : " text-[#CDCCCC] hover:text-[#402D28]" }font-medium text-lg leading-5 cursor-pointer border-[#CDCCCC] border-b md:border-none md:py-0 py-6 md:my-3 block w-full`} key={index} onClick={() => router.push(`/identities/${domain.id}`)} > {domain.id} </button>
222-232: Add loading state for mint operation.The "ADD IDENTITIES" button should show a loading state during the minting process to prevent multiple clicks.
<button className="bottom-4 w-full justify-center text-center items-center font-quickZap font-normal flex gap-2" + disabled={minting} onClick={ address ? () => mint() : () => setShowWalletConnectModal(true) } > - <FaPlus /> + {minting ? ( + <span className="animate-spin">↻</span> + ) : ( + <FaPlus /> + )} ADD IDENTITIES </button>
172-180: Remove or document commented code.The commented-out code for fetching external domains should either be removed if it's no longer needed or documented with a TODO comment explaining why it's kept for future use.
- // fetch( - // `${ - // process.env.NEXT_PUBLIC_SERVER_LINK - // }/addr_to_external_domains?addr=${hexToDecimal(address)}` - // ) - // .then((response) => response.json()) - // .then((data: ExternalDomains) => { - // setExternalDomains(data.domains); - // }); + // TODO: External domains feature to be implemented in future sprint + // Keeping this code for reference
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pages/identities/[tokenId].tsx(6 hunks)
🔇 Additional comments (3)
pages/identities/[tokenId].tsx (3)
9-13: LGTM! New imports are properly organized.The added imports from @starknet-react/core and other utilities align well with the new wallet connection and transaction functionality.
Also applies to: 23-27
52-62: LGTM! Transaction setup is properly memoized.The contract address is correctly accessed from environment variables and the callData is properly memoized.
47-50: 🛠️ Refactor suggestionConsider using a more robust method for generating random token IDs.
Using
Math.random()for token ID generation could potentially lead to collisions. Consider using a more cryptographically secure random number generator or implementing a proper token ID allocation strategy.- const randomTokenId: number = Math.floor(Math.random() * 1000000000000); + const randomTokenId: number = BigInt( + "0x" + crypto.getRandomValues(new Uint8Array(8)).reduce((s, b) => s + b.toString(16).padStart(2, '0'), '') + ).toString();
There was a problem hiding this comment.
Well done, it looks good and it works well! There are a few changes to do
In the list, show the domain if available instead of the id
Instead of showing the identity list in https://app-starknet-g48puevic-lfglabs.vercel.app/identities you should show your new menu, with the first identity selected by default
On figma the shadow is not the same, and you forgot the small border
There is more padding on figma
|
@Marchand-Nicolas, please how can i get the domain name for each mint domain, currently am able to get only the id of the mint domain, |
Hello, I answered on telegram :) |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
pages/identities/[tokenId].tsx (1)
196-208: Add loading state for owned identities list.The list of owned identities lacks a loading state, which could lead to a jarring user experience while data is being fetched.
+ const [isLoadingIdentities, setIsLoadingIdentities] = useState(true); useEffect(() => { if (address) { + setIsLoadingIdentities(true); fetch(...) .then((data) => { setOwnedIdentities(data.full_ids); + setIsLoadingIdentities(false); }) .catch((error) => { console.error('Failed to fetch owned identities:', error); setOwnedIdentities([]); + setIsLoadingIdentities(false); }); } }, [address, router.asPath]); // In the JSX <div className="h-[280px] md:h-[480px] overflow-y-auto"> + {isLoadingIdentities ? ( + <div className="text-center py-4">Loading...</div> + ) : ( {ownedIdentities.map((domain, index) => ( // ... existing code ))} + )} </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pages/identities.tsx(1 hunks)pages/identities/[tokenId].tsx(6 hunks)
🔇 Additional comments (2)
pages/identities/[tokenId].tsx (2)
172-178: Improve error handling in wallet connection.The wallet connection has the same issues as previously identified:
- Using @ts-ignore is not recommended
- localStorage access should be wrapped in try-catch
267-272: LGTM: WalletConnect integration looks good.The WalletConnect component is properly integrated with the necessary props.
pages/identities/[tokenId].tsx
Outdated
| const callData = useMemo(() => { | ||
| return { | ||
| contractAddress: process.env.NEXT_PUBLIC_IDENTITY_CONTRACT as string, | ||
| entrypoint: "mint", | ||
| calldata: [randomTokenId.toString()], | ||
| }; | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, []); | ||
| const { sendAsync: execute } = useSendTransaction({ | ||
| calls: [callData], | ||
| }); |
There was a problem hiding this comment.
Add error handling for transaction execution.
The mint function lacks error handling, which could lead to unhandled promise rejections.
- function mint() {
- execute();
+ async function mint() {
+ try {
+ await execute();
+ } catch (error) {
+ console.error('Failed to execute mint transaction:', error);
+ // Consider showing an error notification to the user
+ }
}Committable suggestion skipped: line range outside the PR's diff.
pages/identities.tsx
Outdated
| @@ -95,6 +95,12 @@ const Identities: NextPage = () => { | |||
| execute(); | |||
| } | |||
|
|
|||
| useEffect(()=>{ | |||
There was a problem hiding this comment.
Instead of redirecting the user, wrap your code from pages/identities/[tokenId].tsx in a component you can reuse in both files
pages/identities/[tokenId].tsx
Outdated
| @@ -16,6 +20,11 @@ import { formatHexString } from "../../utils/stringService"; | |||
| import { getDomainData } from "@/utils/cacheDomainData"; | |||
| import { useSearchParams } from "next/navigation"; | |||
| import IdentityActionsSkeleton from "@/components/identities/skeletons/identityActionsSkeleton"; | |||
| import { hexToDecimal } from "@/utils/feltService"; | |||
| import AddButton from "@/components/UI/AddButtonIdentities"; | |||
There was a problem hiding this comment.
You can remove this import, it is not used
There was a problem hiding this comment.
Oops miss click sorry I mean't to request these changes @blessingbytes
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
components/identities/availableIdentities.tsx (3)
158-168:⚠️ Potential issueImprove error handling in fetch operation.
The fetch operation for owned identities lacks proper error handling.
170-176:⚠️ Potential issueImprove error handling in wallet connection.
The wallet connection code needs proper typing and error handling for localStorage access.
152-154:⚠️ Potential issueAdd error handling for transaction execution.
The mint function needs proper error handling.
🧹 Nitpick comments (7)
components/identities/availableIdentities.tsx (5)
1-25: Consider adding a Props interface for better type safety.Define an explicit interface for the component's props to improve maintainability and type safety.
+interface AvailableIdentitiesProps { + tokenId: string; +} + -const AvailableIdentities = ({ tokenId }: { tokenId: string }) => { +const AvailableIdentities = ({ tokenId }: AvailableIdentitiesProps) => {Also applies to: 26-27
29-29: Remove commented code.Remove the commented line as it's no longer needed.
- //const tokenId: string = router.query.tokenId as string;
44-45: Add type definition for FullId and move random ID generation to a utility function.The
FullIdtype is not defined, and the random token ID generation logic could be extracted for better maintainability.+interface FullId { + id: string; + domain?: string; +} + +const generateRandomTokenId = (): number => Math.floor(Math.random() * 1000000000000); + const [ownedIdentities, setOwnedIdentities] = useState<FullId[]>([]); -const randomTokenId: number = Math.floor(Math.random() * 1000000000000); +const randomTokenId: number = generateRandomTokenId();
137-150: Consider making polling intervals configurable.The refresh intervals (1s for minting and 30s for regular updates) should be configurable constants.
+const MINTING_POLL_INTERVAL = 1000; // 1 second +const REGULAR_POLL_INTERVAL = 30000; // 30 seconds + useEffect(() => { if (minting && tokenId && !identity) { - const interval = setInterval(() => refreshData(), 1000); + const interval = setInterval(() => refreshData(), MINTING_POLL_INTERVAL); return () => clearInterval(interval); } }, [minting, tokenId, identity, refreshData]); useEffect(() => { if (tokenId) { refreshData(); - const timer = setInterval(() => refreshData(), 30e3); + const timer = setInterval(() => refreshData(), REGULAR_POLL_INTERVAL); return () => clearInterval(timer); } }, [tokenId, refreshData]);
183-245: Consider extracting complex UI logic into separate components.The nested conditional rendering and complex class name logic could be simplified by extracting them into separate components.
Consider creating:
IdentityListcomponent for the list of owned identitiesMintButtoncomponent for the add identities button- Moving inline styles to CSS modules
This would improve readability and maintainability.
pages/identities/[tokenId].tsx (2)
15-15: Replace window.history.back() with router navigation.Using
window.history.back()can lead to unexpected behavior. Consider using Next.js router for consistent navigation.-<BackButton onClick={() => window.history.back()} /> +<BackButton onClick={() => router.back()} />
8-21: Consider adding error boundaries.The component should handle potential errors from the AvailableIdentities component gracefully.
+import ErrorBoundary from '@/components/ErrorBoundary'; const TokenIdPage: NextPage = () => { const router = useRouter(); const tokenId: string = router.query.tokenId as string; return ( <div className={homeStyles.wrapperScreen}> <div className="mt-24"> <div className="ml-8 mb-5"> <BackButton onClick={() => router.back()} /> </div> + <ErrorBoundary fallback={<div>Something went wrong</div>}> <AvailableIdentities tokenId={tokenId} /> + </ErrorBoundary> </div> </div> ); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
components/identities/availableIdentities.tsx(1 hunks)pages/identities.tsx(3 hunks)pages/identities/[tokenId].tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pages/identities.tsx
close #944
Summary by CodeRabbit
New Features
Improvements