feat: add logout feature + fix layout #641
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis pull request introduces several significant changes across multiple files in the mobile UI section of the application. The modifications focus on improving navigation, layout, and user experience by dynamically rendering page titles, handling empty states, and refining the overall application structure. Key changes include adding a new Changes
Possibly related PRs
Suggested reviewers
Poem
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
🔭 Outside diff range comments (1)
src/components/Home/HomeWaitlist.tsx (1)
Line range hint
67-89: Remove commented-out code.Since the login/register components have been moved to a different page, the commented-out code should be removed to maintain code cleanliness.
- {/* removed regiser/login components. Should be in diff page. */} - {/* : ( - <div className="flex flex-col items-center justify-center"> - <Button - onClick={() => { - push('/setup') - }} - shadowSize="4" - > - Register - </Button> - <Divider text="or" /> - <Button - loading={isLoggingIn} - disabled={isLoggingIn} - onClick={() => { - handleLogin() - }} - variant="stroke" - > - Login - </Button> - </div> */}
🧹 Nitpick comments (8)
src/components/0_Bruddle/PageContainer.tsx (1)
4-4: Consider using more specific child selectors.While the arbitrary value selector
*:w-fullworks, it might have unintended consequences by affecting all child elements. Consider using more specific selectors or a dedicated class for direct children.- return <div className="flex w-full items-center justify-center *:w-full md:*:w-2/5">{props.children}</div> + return <div className="flex w-full items-center justify-center"> + <div className="w-full md:w-2/5">{props.children}</div> + </div>src/components/Global/TopNavbar/index.tsx (1)
21-30: Consider adding a confirmation dialog before logout.To prevent accidental logouts, consider adding a confirmation dialog when the user clicks the logout button.
<Button disabled={isLoggingOut} size="medium" variant="transparent-dark" - onClick={logout} + onClick={() => { + if (window.confirm('Are you sure you want to logout?')) { + logout() + } + }} className="flex w-fit items-center gap-3 hover:text-gray-1" >src/constants/general.consts.ts (1)
188-198: Improve consistency and coverage of path titles.The
pathTitlesmapping could be enhanced in the following ways:
- Use consistent title casing (e.g., 'Wallet Asset' instead of 'Wallet asset')
- Add missing common paths that appear in the codebase (e.g., '/setup', '/topup')
export const pathTitles: { [key: string]: string } = { '/home': 'Dashboard', '/send': 'Send', - '/wallet': 'Wallet asset', + '/wallet': 'Wallet Asset', '/request/create': 'Request Payment', '/request/pay': 'Pay Request', '/cashout': 'Cashout', '/history': 'History', '/support': 'Support', '/claim': 'Claim Payment', + '/setup': 'Setup Wallet', + '/topup': 'Top Up', }src/app/(mobile-ui)/layout.tsx (2)
38-41: Simplify pathname checks using pathTitles.Instead of individual boolean flags for each path, consider using the existing
pathTitlesmapping to determine the alignment behavior.- const isHistory = pathName === '/history' - const isWallet = pathName === '/wallet' - const isSupport = pathName === '/support' - const alignStart = isHome || isHistory || isWallet || isSupport + const alignStartPaths = ['/home', '/history', '/wallet', '/support'] + const alignStart = alignStartPaths.includes(pathName)
73-78: Use consistent class merging utility.The code uses both
classNamesandtwMergeutilities. Consider using justtwMergefor consistency.- className={classNames( - twMerge( - 'flex-1 overflow-y-auto bg-background p-6 pb-24 md:pb-6', - !!isSupport && 'p-0 pb-20 md:p-6' - ) - )} + className={twMerge( + 'flex-1 overflow-y-auto bg-background p-6 pb-24 md:pb-6', + !!isSupport && 'p-0 pb-20 md:p-6' + )}src/app/(mobile-ui)/history/page.tsx (1)
130-130: Simplify conditional rendering and maintain consistent border styling.
- Remove redundant double negation in the NavHeader rendering
- Consider moving the border styling to the item level for consistency
- {!!data?.pages.length ? <NavHeader title={getHeaderTitle(pathname)} /> : null} + {data?.pages.length > 0 && <NavHeader title={getHeaderTitle(pathname)} />} <div className="h-full w-full"> - {!!data?.pages.length && + {data?.pages.length > 0 && data?.pages.map((page, pageIndex) => ( - <div key={pageIndex} className="border-b border-n-1"> + <div key={pageIndex}>Also applies to: 134-134
🧰 Tools
🪛 Biome (1.9.4)
[error] 130-130: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
src/context/authContext.tsx (1)
86-86: Consider the timing between navigation and toast notification.The logout implementation is good, but there's a potential race condition where the user might not see the success toast if the navigation happens too quickly.
Consider this alternative implementation:
const logoutUser = async () => { if (isLoggingOut) return setIsLoggingOut(true) try { const response = await fetch('/api/peanut/user/logout-user', { method: 'GET', headers: { 'Content-Type': 'application/json', }, }) if (response.ok) { localStorage.removeItem(LOCAL_STORAGE_WEB_AUTHN_KEY) await fetchUser() + // Show toast first + toast({ + status: 'success', + title: 'Logged out successfully', + duration: 3000, + }) + // Add a small delay to ensure toast is visible + await new Promise(resolve => setTimeout(resolve, 500)) router.push('/setup') - toast({ - status: 'success', - title: 'Logged out successfully', - duration: 3000, - }) } else { console.error('Failed to log out user') toast({ status: 'error', title: 'Failed to log out', description: 'Please try again', duration: 5000, }) } } catch (error) { console.error('Error logging out user', error) toast({ status: 'error', title: 'Error logging out', description: 'Please try again', duration: 5000, }) } finally { setIsLoggingOut(false) } }Also applies to: 264-303
src/utils/general.utils.ts (1)
1074-1076: Add return type for better type safety.The function implementation is good, but could benefit from explicit typing.
Consider this improvement:
-export const getHeaderTitle = (pathname: string) => { +export const getHeaderTitle = (pathname: string): string => { return consts.pathTitles[pathname] || 'Peanut Protocol' }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/assets/icons/logout.svgis excluded by!**/*.svg
📒 Files selected for processing (12)
src/app/(mobile-ui)/history/page.tsx(3 hunks)src/app/(mobile-ui)/home/page.tsx(1 hunks)src/app/(mobile-ui)/layout.tsx(4 hunks)src/app/(mobile-ui)/wallet/page.tsx(2 hunks)src/assets/icons/index.ts(1 hunks)src/components/0_Bruddle/PageContainer.tsx(1 hunks)src/components/Global/TopNavbar/index.tsx(1 hunks)src/components/Global/WalletNavigation/index.tsx(1 hunks)src/components/Home/HomeWaitlist.tsx(1 hunks)src/constants/general.consts.ts(2 hunks)src/context/authContext.tsx(7 hunks)src/utils/general.utils.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/components/Global/WalletNavigation/index.tsx
- src/app/(mobile-ui)/home/page.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
src/app/(mobile-ui)/history/page.tsx
[error] 130-130: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
🔇 Additional comments (6)
src/assets/icons/index.ts (1)
10-10: LGTM!The new LOGOUT_ICON export follows the established pattern and maintains alphabetical ordering.
src/components/Home/HomeWaitlist.tsx (1)
Line range hint
67-67: Verify the new location of login/register components.The comment suggests these components should be in a different page. Let's verify their new location.
src/app/(mobile-ui)/wallet/page.tsx (1)
21-21: Consider desktop navigation title.While the mobile view correctly uses NavHeader with a dynamic title, there's no equivalent title display for desktop view. This might create inconsistency in the user experience across different screen sizes.
Let's check if the desktop title is handled by the new TopNavbar component:
Also applies to: 46-46
src/app/(mobile-ui)/history/page.tsx (1)
110-126: Good addition of empty state handling!The NoDataEmptyState component with a clear message and call-to-action button improves the user experience when there are no transactions.
src/context/authContext.tsx (2)
37-37: LGTM! Good addition of loading state.The
isLoggingOutstate is a good practice for tracking the logout operation's progress and preventing duplicate attempts.
9-9: LGTM! Correct import for Next.js navigation.Using
next/navigationis the recommended approach for routing in Next.js applications.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/app/(mobile-ui)/request/route.ts (1)
1-5: Consider enhancing type safety and error handling.While the implementation is functional, consider these improvements:
- Convert to TypeScript for better type safety
- Add error handling for edge cases
Here's a suggested implementation:
-import { redirect } from 'next/navigation' +import { redirect } from 'next/navigation' +import { NextRequest } from 'next/server' -export function GET() { +export async function GET(request: NextRequest) { + try { redirect('/request/create') + } catch (error) { + console.error('Error redirecting to request/create:', error) + throw error + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/app/(mobile-ui)/request/route.ts(1 hunks)
🔇 Additional comments (1)
src/app/(mobile-ui)/request/route.ts (1)
3-5: Verify desktop navigation behavior.Since this route handler is in the mobile-ui directory but performs an unconditional redirect, verify that this doesn't interfere with desktop navigation patterns.
Let's check for any desktop-specific routes or components that might be affected:
✅ Verification successful
Redirect implementation is correct and consistent with application architecture.
The redirect from
/requestto/request/createis appropriate as:
- Both mobile and desktop views use
/request/createas the main request page- The pattern is consistently used across navigation components
- No separate desktop-specific routes exist that could be affected
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for desktop-specific routes or components that might use the /request path rg -g '!node_modules' -l '(desktop|Desktop).*request' # Check if there are any other route handlers for /request fd -g '**/request/route.ts' -E 'node_modules'Length of output: 101
Script:
#!/bin/bash # Check the app directory structure to understand routing organization fd --type directory --max-depth 3 --base-directory src/app # Look for any route files that might handle /request path fd route.ts --type file --base-directory src/app # Search for references to the request path rg -g '!node_modules' -g '!*.{json,md}' '/request(/|"|'"'"')'Length of output: 4397
| } | ||
|
|
||
| export const getHeaderTitle = (pathname: string) => { | ||
| return consts.pathTitles[pathname] || 'Peanut Protocol' // default title if path not found |
There was a problem hiding this comment.
qn: should use peanut protocol as fallback or something else?
There was a problem hiding this comment.
I think it's a good fallback
| } | ||
|
|
||
| export const getHeaderTitle = (pathname: string) => { | ||
| return consts.pathTitles[pathname] || 'Peanut Protocol' // default title if path not found |
There was a problem hiding this comment.
I think it's a good fallback
@jjramirezn , dont have design mockups for it yet, i think its non blocking and can be done later on, will add a task to track this |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/components/Global/LogoutButton/index.tsx (1)
14-25: Enhance accessibility and error handling.The button implementation looks good but could benefit from improved accessibility and error handling.
Apply these changes:
<Button disabled={isLoggingOut} size="medium" variant="transparent-dark" onClick={logout} + aria-label="Logout from application" className="flex w-fit items-center gap-3 hover:text-gray-1" + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + logout(); + } + }} >src/context/authContext.tsx (2)
262-263: Move constants to a dedicated configuration file.The
LOCAL_STORAGE_WEB_AUTHN_KEYconstant should be moved to a dedicated configuration file for better maintainability and reusability.Create a new file
src/config/constants.ts:export const AUTH_CONSTANTS = { WEB_AUTHN_KEY: 'web-authn-key', // Add other auth-related constants here } as const;Then import and use it in this file:
-const LOCAL_STORAGE_WEB_AUTHN_KEY = 'web-authn-key' +import { AUTH_CONSTANTS } from '@/config/constants'
Line range hint
264-307: Enhance error handling and add retry mechanism.The error handling could be improved with more specific error messages and a retry mechanism for transient failures.
const logoutUser = async () => { if (isLoggingOut) return setIsLoggingOut(true) + let retryCount = 0; + const MAX_RETRIES = 3; + + const attemptLogout = async () => { try { const response = await fetch('/api/peanut/user/logout-user', { method: 'GET', headers: { 'Content-Type': 'application/json', }, }) if (response.ok) { - localStorage.removeItem(LOCAL_STORAGE_WEB_AUTHN_KEY) + localStorage.removeItem(AUTH_CONSTANTS.WEB_AUTHN_KEY) // clear JWT cookie by setting it to expire document.cookie = 'jwt-token=; path=/; expires=Thu, 01 Jan 1970 00:00:01 GMT;' await fetchUser() router.push('/setup') toast({ status: 'success', title: 'Logged out successfully', duration: 3000, }) } else { - console.error('Failed to log out user') + const errorData = await response.json().catch(() => null); + const errorMessage = errorData?.message || 'Unknown error occurred'; + console.error('Failed to log out user:', errorMessage); + + if (retryCount < MAX_RETRIES && response.status >= 500) { + retryCount++; + await new Promise(resolve => setTimeout(resolve, 1000 * retryCount)); + return attemptLogout(); + } + toast({ status: 'error', title: 'Failed to log out', - description: 'Please try again', + description: `Please try again. Error: ${errorMessage}`, duration: 5000, }) } } catch (error) { - console.error('Error logging out user', error) + console.error('Error logging out user:', error instanceof Error ? error.message : error) + + if (retryCount < MAX_RETRIES) { + retryCount++; + await new Promise(resolve => setTimeout(resolve, 1000 * retryCount)); + return attemptLogout(); + } + toast({ status: 'error', title: 'Error logging out', - description: 'Please try again', + description: error instanceof Error ? error.message : 'Please try again', duration: 5000, }) } finally { setIsLoggingOut(false) } + } + + return attemptLogout(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/app/(mobile-ui)/home/page.tsx(3 hunks)src/components/Global/LogoutButton/index.tsx(1 hunks)src/components/Global/TopNavbar/index.tsx(1 hunks)src/context/authContext.tsx(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/Global/TopNavbar/index.tsx
- src/app/(mobile-ui)/home/page.tsx
🔇 Additional comments (1)
src/context/authContext.tsx (1)
276-279: Verify secure cleanup of authentication data.The implementation correctly removes both the web-authn key and JWT token as suggested in the past review. However, consider adding additional security measures.
Run this script to check for any other authentication-related data that might need cleanup:
#!/bin/bash # Search for other potential authentication-related storage or cookie operations echo "Searching for localStorage operations..." rg "localStorage\." -A 2 -B 2 echo "Searching for cookie operations..." rg "document\.cookie" -A 2 -B 2 echo "Searching for other potential auth tokens..." rg -i "(token|auth|jwt|credential)" -g "!*.md" -g "!*.json"
@jjramirezn added a logout btn, improvised the ui for now, can revisit when have proper mockups |
/request/createif on/requestfixes TASK-8154
fixes TASK-8267
fixes TASK-8307
fixes TASK-8403
Summary by CodeRabbit
Summary by CodeRabbit
Based on the comprehensive summary, here are the updated release notes:
New Features
User Interface
Authentication
Performance
Chores