Fix: click on my identities to go back#1176
Fix: click on my identities to go back#1176Marchand-Nicolas merged 4 commits intolfglabs-dev:testnetfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
""" WalkthroughA new Changes
Assessment against linked issues
Suggested labels
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ 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. 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 (
|
|
@Verifieddanny is attempting to deploy a commit to the LFG Labs Team on Vercel. A member of the Team first needs to authorize it. |
![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
🧹 Nitpick comments (1)
components/UI/navbar.tsx (1)
90-92: Apply optional chaining for cleaner code.The static analysis tool correctly identified that this can be simplified using optional chaining.
- const connector = getLastConnected() - if (connector && connector.available()) await connectWallet(connector) + const connector = getLastConnected() + if (connector?.available()) await connectWallet(connector)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/UI/navbar.tsx(12 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
components/UI/navbar.tsx
[error] 91-91: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (4)
components/UI/navbar.tsx (4)
142-150: Excellent fix for the mobile navigation timing issue!The
handleMobileNavClickfunction properly addresses the race condition between menu closing and navigation. The 100ms delay ensures the menu closes before navigation occurs, preventing the routing conflicts.
293-334: Mobile navigation links successfully converted to use the new handler.The conversion from
<Link>components withonClickhandlers to<li>elements with the newhandleMobileNavClickfunction is correctly implemented. This ensures consistent behavior across all mobile navigation links and external links.
218-218: Good defensive programming with fallback image source.Adding the fallback to
/placeholder.svgprevents broken images when profile pictures are undefined.
233-233: Proper fallback handling for connector icons.The fallback value ensures the UI remains functional when connector icons are unavailable.
Marchand-Nicolas
left a comment
Marchand-Nicolas
left a comment
There was a problem hiding this comment.
It doesn't build
|
@Marchand-Nicolas Issue Fixed |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
components/UI/navbar.tsx (2)
57-104: LGTM: Effect hooks are properly implementedThe effect hooks maintain their logic correctly. The new effect for updating
lastConnectorstate is appropriate.However, there's a static analysis suggestion to improve line 92:
- if (connector && connector.available()) await connectWallet(connector) + if (connector?.available()) await connectWallet(connector)
143-151: LGTM: Effective solution for the mobile navigation timing issueThe
handleMobileNavClickfunction properly addresses the race condition by closing the mobile menu before navigation. The 100ms delay ensures the menu state update completes before routing occurs.Consider adding error handling for the navigation:
const handleMobileNavClick = (href: string) => { // Close the mobile menu first setNav(false) // Small delay to ensure menu closes before navigation setTimeout(() => { - router.push(href) + router.push(href).catch(console.error) }, 100) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/UI/navbar.tsx(12 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
components/UI/navbar.tsx
[error] 92-92: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (6)
components/UI/navbar.tsx (6)
1-31: LGTM: Clean import organization and explicit type annotationsThe import statements are well-organized and the explicit type annotations for
FunctionComponent,StarkProfile, andConnectorimprove code clarity.
33-63: LGTM: Proper state management with explicit typingThe state declarations are well-structured with explicit TypeScript annotations. The new
lastConnectorstate variable is properly typed and initialized.
106-141: LGTM: Event handlers are well-formatted and maintain correct logicThe event handlers are properly structured with consistent formatting. The logic remains intact and functions are correctly typed.
176-254: LGTM: Desktop navigation improvements with proper fallbacksThe desktop navigation maintains its
<Link>component approach, which is correct since the timing issue was mobile-specific. The addition of fallback placeholders for profile pictures and connector icons improves robustness.
293-335: LGTM: Comprehensive mobile navigation fix addressing the core issueThe mobile navigation implementation correctly addresses the race condition by:
- Replacing
<Link>components with<li>elements usingonClickhandlers- Using
handleMobileNavClickfor internal navigation (identities, domains, pfpcollections, newsletter)- Properly handling external links with
window.openand menu closure- Ensuring the mobile menu closes before any navigation action
This implementation directly fixes the issue described in the PR objectives where users couldn't navigate back to "My Identities" from the PFP page.
336-401: LGTM: Component structure and exports are properly maintainedThe remaining component structure, including modals and social links, is appropriately unchanged. The export statement is correctly formatted.
|
@Marchand-Nicolas Please Kindly check the updates |
components/UI/navbar.tsx
Outdated
|
|
||
| // Fixed function to handle mobile navigation clicks | ||
| const handleMobileNavClick = (href: string) => { | ||
| // Close the mobile menu first |
There was a problem hiding this comment.
Can you cancel those changes, this is not the issue.
When you are on /identities (not /identities/xxx), if you edit your pfp and click "My identities", nothing will happen (because you are already on /identities.)
There was a problem hiding this comment.
ohh my bad, on it now
… fix the current issue
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
components/identities/availableIdentities.tsx (1)
207-213: Effect works; tiny optimisation possibleThe hook correctly exits PFP-update mode when the user lands on
/identities.
If you want to avoid an extra render, guard the setter so it only fires when needed:- if (router.pathname === "/identities" && !router.query.tokenId) { - setIsUpdatingPp(false) - } + if ( + router.pathname === "/identities" && + !router.query.tokenId && + isUpdatingPp + ) { + setIsUpdatingPp(false); + }Purely optional – current code is fine.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
components/UI/navbar.tsx(1 hunks)components/identities/availableIdentities.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- components/UI/navbar.tsx
Marchand-Nicolas
left a comment
Marchand-Nicolas
left a comment
There was a problem hiding this comment.
It's still not working, have you tested ?
If you need mroe details or need help to test please ask
Marchand-Nicolas
left a comment
Marchand-Nicolas
left a comment
There was a problem hiding this comment.
Working perfectly well done! Lgtm :D
2 participants
Fix Navigation Issue - Cannot Return to "My Identities" from PFP Page
Problem Description
Users were unable to navigate back to the "My Identities" page when clicking the navbar link while on the PFP page. The link appeared unresponsive, trapping users on the PFP page without a way to return to the main identities view.
fixes #1126
Root Cause Analysis
The issue was caused by a timing conflict in the mobile navigation system:
<Link>component was wrapped with anonClick={handleNav}handlerhandleNav()function was closing the mobile menu simultaneously with navigationThe conflict prevented the router from completing the navigation to
/identities, leaving users stuck on the current PFP page.Solution Implemented
Key Changes:
handleMobileNavClickfunction that properly sequences operations:Replaced problematic
<Link>+onClickpattern in mobile navigation:❌ Before:
<Link href="/identities"><li onClick={handleNav}>My Identities</li></Link>✅ After:
<li onClick={() => handleMobileNavClick("/identities")}>My Identities</li>Fixed all mobile navigation links:
🎯 "My Identities" →
/identities🏠 "Domains" →
/🖼️ "PFP collections" →
/pfpcollections📧 "Newsletter" →
/newsletterImproved external link handling:
🌐 Uses
window.open()for external URLs📱 Properly closes mobile menu before opening external links
Files Modified:
components/navbar.tsx- Fixed mobile navigation timing and routing logicTesting Scenarios
Impact
Summary by CodeRabbit