feat: clear localStorage when logout#2446
Conversation
wilsonrivera
requested review from
JivusAyrus,
StarpTech and
thisisnithin
as code owners
WalkthroughAdds logout cleanup by clearing specific localStorage keys and resetting tracking via a new LogoutLink component; removes Koala integration and all Koala-related calls from the tracking module. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2446 +/- ##
========================================
- Coverage 1.49% 1.49% -0.01%
========================================
Files 292 292
Lines 46968 46983 +15
Branches 431 431
========================================
Hits 703 703
- Misses 45982 45997 +15
Partials 283 283
🚀 New features to boost your workflow:
|
![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
🤖 Fix all issues with AI agents
In @studio/src/components/user-menu.tsx:
- Around line 112-114: The DropdownMenuItem should use Radix's asChild
composition instead of being wrapped by Link to avoid event conflicts; update
the instances where a Link component (e.g., LogoutLink and the other
Login/Account link earlier) currently wrap DropdownMenuItem so that
DropdownMenuItem receives the asChild prop and the Link is rendered inside it
(i.e., change from
<LogoutLink><DropdownMenuItem>…</DropdownMenuItem></LogoutLink> to using
DropdownMenuItem asChild with <LogoutLink> as its child), keeping the same
labels and props.
In @studio/src/lib/track.ts:
- Line 20: The code was only referencing the reset property (window.ko?.reset)
instead of invoking it; change the call to execute the function by using
window.ko?.reset() so the Koala reset runs when available (keep the optional
chaining as shown).
🧹 Nitpick comments (2)
studio/src/components/user-menu.tsx (2)
29-38: Remove or replace the debug console.log statement.Line 35 logs each removed key to the console. This appears to be debug code that should either be removed before production or replaced with a proper logging mechanism if diagnostics are needed.
🧹 Proposed cleanup
for (const key of localStorageKeysToRemove) { - console.log(key); window.localStorage.removeItem(key); }
40-52: Consider adding error handling for cleanup operations.The onClick handler calls
removeLocalStorageItems()andresetTracking()without error handling. If either operation throws an exception, the logout navigation could be interrupted. Consider wrapping these calls in a try-catch block to ensure logout proceeds even if cleanup fails.🛡️ Proposed defensive handling
<Link onClick={() => { - removeLocalStorageItems(); - resetTracking(); + try { + removeLocalStorageItems(); + resetTracking(); + } catch (error) { + console.error('Failed to clear user data on logout:', error); + // Allow logout to proceed regardless + } }} href={process.env.NEXT_PUBLIC_COSMO_CP_URL + "/v1/auth/logout"} >
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
studio/src/components/user-menu.tsxstudio/src/lib/track.ts
🧰 Additional context used
🧬 Code graph analysis (1)
studio/src/components/user-menu.tsx (2)
studio/src/lib/track.ts (1)
resetTracking(76-76)studio/src/components/ui/dropdown-menu.tsx (1)
DropdownMenuItem(206-206)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
studio/src/components/user-menu.tsx (2)
14-27: LGTM: Import and localStorage keys defined appropriately.The PropsWithChildren import is correctly used for the LogoutLink component, and the array of localStorage keys targets GraphiQL and Playground-related state that should be cleared on logout.
63-63: Fix nested interactive elements causing accessibility violation.Nesting
<LogoutLink>(which renders a<Link>) inside a<Button>creates invalid HTML and breaks accessibility. Interactive elements should not be nested. This causes issues with screen readers, keyboard navigation, and event handling.♿ Proposed fix
Replace the Button wrapping the LogoutLink with the Button's styling applied directly to the LogoutLink:
- <Button> - <LogoutLink /> - </Button> + <LogoutLink> + <Button asChild>Logout</Button> + </LogoutLink>Or alternatively, if Button supports
asChildpattern, apply the button styling to the Link:- <Button> - <LogoutLink /> - </Button> + <Button asChild> + <LogoutLink /> + </Button>Likely an incorrect or invalid review comment.
…-not-clear-cookies-and-local
…-not-clear-cookies-and-local
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@studio/src/components/user-menu.tsx`:
- Around line 61-63: The Button wrapper currently contains LogoutLink (which
renders an <a>), causing invalid nested interactive elements; update the usage
so the link is the interactive element: either use the Button component's
asChild prop (if available) and render LogoutLink as the child (ensure
LogoutLink forwards refs and accepts className/props), or remove the Button
wrapper and apply Button's styles/classes directly to LogoutLink; change the JSX
around Button and LogoutLink accordingly and adjust LogoutLink to forwardRef and
accept extra props when using asChild.
♻️ Duplicate comments (1)
studio/src/components/user-menu.tsx (1)
111-113: UseasChildprop for proper Link composition with DropdownMenuItem.This wraps
DropdownMenuIteminsideLogoutLink(aLinkcomponent), which causes event handling conflicts. The Radix UI dropdown menu primitive supports theasChildprop for proper composition.Suggested fix
- <LogoutLink> - <DropdownMenuItem>Logout</DropdownMenuItem> - </LogoutLink> + <DropdownMenuItem asChild> + <LogoutLink>Logout</LogoutLink> + </DropdownMenuItem>Note:
LogoutLinkwill need to forward refs for this pattern to work correctly:const LogoutLink = React.forwardRef<HTMLAnchorElement, PropsWithChildren<{ className?: string }>>( ({ children, ...props }, ref) => { return ( <Link ref={ref} onClick={() => { removeLocalStorageItems(); resetTracking(); }} href={process.env.NEXT_PUBLIC_COSMO_CP_URL + "/v1/auth/logout"} {...props} > {children || "Logout"} </Link> ); } ); LogoutLink.displayName = "LogoutLink";
…-not-clear-cookies-and-local
…-not-clear-cookies-and-local
…-not-clear-cookies-and-local
…-not-clear-cookies-and-local
wilsonrivera
deleted the
wilson/eng-8605-wun-q425-09-logging-out-does-not-clear-cookies-and-local
branch
3 participants
Summary by CodeRabbit
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.
Checklist