fix(navigation): prevent full reload when clicking profile avatar (#636) #650
Conversation
WalkthroughReplaced the avatar's anchor link with React Router's Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Navbar
participant Router
participant Settings
User->>Navbar: click avatar
activate Navbar
Navbar->>Router: useNavigate("/settings")
Router->>Settings: load Settings component (client-side)
deactivate Navbar
Note over Router,Settings: No full page reload
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
frontend/src/components/Navigation/Navbar/Navbar.tsx (2)
25-28: Consider updating the logo link for consistency.While outside the scope of this PR, note that the logo link on line 25 still uses
<a href="/">, which will cause the same full-reload behavior this PR aims to fix. For consistency and a fully seamless SPA experience, consider converting this to usenavigate("/")or React Router's<Link>component in a future update.
9-9: Consider standardizing imports to"react-router-dom"for consistency and best practices.While
import { useNavigate } from "react-router"is technically valid in React Router v6, the codebase mixes imports:Linkis imported from"react-router-dom"whileuseNavigateuses"react-router". For web applications (especially with Tauri), the standard and recommended pattern is to import both from"react-router-dom". This import pattern appears across multiple files in the codebase; consider standardizing all useNavigate imports to use"react-router-dom"for consistency.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/src/components/Navigation/Navbar/Navbar.tsx (2)
87-97: Button implementation looks good with one minor nitpick.The button correctly addresses the accessibility concerns raised in previous reviews and implements client-side navigation as intended. The
aria-labelprovides good accessibility, and the semantic<button>element ensures keyboard navigation works properly.Optional cleanup: The
cursor-pointerclass is redundant since buttons havecursor: pointerby default in browsers. You can simplify the classes:- className="p-2 cursor-pointer border-0 bg-transparent" + className="p-2 border-0 bg-transparent"
25-28: Consider applying the same navigation pattern to the logo for consistency.The logo still uses an anchor tag with
href="/", which will trigger a full page reload (the same issue this PR fixes for the avatar). For a consistent SPA experience without white-screen flashes, consider using client-side navigation here as well.Apply this diff to use consistent client-side navigation:
- <a href="/" className="flex items-center space-x-2"> + <button + onClick={() => navigate("/")} + className="flex items-center space-x-2 border-0 bg-transparent cursor-pointer" + > <img src="/128x128.png" width={32} height={32} alt="PictoPy Logo" /> <span className="text-xl font-bold">PictoPy</span> - </a> + </button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/components/Navigation/Navbar/Navbar.tsx(3 hunks)
🔇 Additional comments (2)
frontend/src/components/Navigation/Navbar/Navbar.tsx (2)
19-19: LGTM!The
useNavigatehook is properly instantiated at the component's top level with correct indentation.
9-9: > Likely an incorrect or invalid review comment.
|
check out the date |
2 participants
What this PR fixes
Fixes the white-screen flash when clicking the profile avatar in the navbar.
The issue was caused by using
<a href="/settings">, which triggers a full page reload in React/Vite/Tauri. This produces a white flash before the settings page loads.What I changed
<a href="/settings">with client-side navigation usinguseNavigate().Related Issue
Closes #636
Checklist
Summary by CodeRabbit
Style
Improvements