🎨 Palette: Improve Login UX and Navbar accessibility#33
🎨 Palette: Improve Login UX and Navbar accessibility#33BilalkhanIO wants to merge 1 commit intomainfrom
Conversation
- Added isLoading state to Login form with 2s simulation delay. - Improved Login form structure by isolating the login button. - Added :disabled styles for primary buttons in Login and Signup CSS. - Enhanced Navbar cart accessibility with aria-label and aria-hidden attributes. - Added UX learnings to .Jules/palette.md. Co-authored-by: BilalkhanIO <48455259+BilalkhanIO@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
📝 WalkthroughWalkthroughThis pull request enhances the Login component with asynchronous loading state management, adds disabled button styling to Login and Signup components, implements accessibility improvements to the Navbar shopping cart link, and documents UX learning notes on ARIA label handling and submit button isolation. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 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
🧪 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: 1
🤖 Fix all issues with AI agents
In `@fy-project/src/components/Login/Login.jsx`:
- Around line 20-28: Remove the console.log that prints email and password
inside handleSubmit to avoid credential exposure, and ensure the simulated
network setTimeout is cleaned up on component unmount: store the timeout id
(from setTimeout called in handleSubmit) in a ref (e.g., loginTimeoutRef), clear
that timeout in a useEffect cleanup (or clear it when handleSubmit finishes) and
guard setIsLoading calls so they only run while mounted (clearing timeout
prevents the post-delay setIsLoading(false)). Update references to handleSubmit,
setIsLoading, and the setTimeout call accordingly and do not log email or
password anywhere.
🧹 Nitpick comments (1)
fy-project/src/components/Navbar/Navbar.jsx (1)
24-26: Derive the aria-label from the same cart count source to avoid drift.Proposed refactor (single source of truth)
function Navbar() { + const cartCount = 3; return ( @@ - <a href='#addToCart' className='navbar__cart' aria-label="View shopping cart, 3 items"> + <a + href='#addToCart' + className='navbar__cart' + aria-label={`View shopping cart, ${cartCount} item${cartCount === 1 ? '' : 's'}`} + > <FiShoppingBag className="navbar__cart-icon" aria-hidden="true" /> - <span className='navbar__cart_span' aria-hidden="true">3</span> + <span className='navbar__cart_span' aria-hidden="true">{cartCount}</span> </a>
| const handleSubmit = (event) => { | ||
| event.preventDefault(); | ||
| // Perform login validation | ||
| console.log("Logging in with email:", email, " and password:", password); | ||
| setIsLoading(true); | ||
|
|
||
| // Simulate a network request | ||
| setTimeout(() => { | ||
| setIsLoading(false); | ||
| console.log("Logging in with email:", email, " and password:", password); | ||
| }, 2000); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n fy-project/src/components/Login/Login.jsx | head -50Repository: BilalkhanIO/FYP
Length of output: 1762
🌐 Web query:
React 18 useEffect cleanup pattern setTimeout memory leak unmount
💡 Result:
In React 18, the safe pattern for setTimeout inside useEffect is:
useEffect(() => {
const id = setTimeout(() => {
// do work (optionally setState)
}, 500);
return () => clearTimeout(id); // runs on unmount + before re-run
}, []);Why this fixes “memory leak / unmount” issues:
- React calls your Effect’s cleanup on unmount and before re-running the Effect when dependencies change, so
clearTimeoutprevents a pending timer from firing after the component is gone. [1] - In React 18 Strict Mode (development only), React intentionally does an extra setup → cleanup → setup cycle to catch missing cleanup—so without
clearTimeout, you can accidentally schedule duplicate timers and/or run work after the “test unmount.” [1][2] - The old warning (“Can’t perform a React state update on an unmounted component…”) was removed because it produced many false positives; you still should clean up real external resources like timers, subscriptions, listeners, etc. [3]
If your timeout callback does async work (or you want extra safety), combine cleanup with a cancel flag:
useEffect(() => {
let cancelled = false;
const id = setTimeout(async () => {
const data = await somethingAsync();
if (!cancelled) setState(data);
}, 500);
return () => {
cancelled = true;
clearTimeout(id);
};
}, []);Sources:
[1] React useEffect reference (setup/cleanup timing; Strict Mode extra cycle) https://react.dev/reference/react/useEffect
[2] React <StrictMode> reference (re-running Effects in dev to find missing cleanup) https://react.dev/reference/react/StrictMode
[3] React 18 working group: removal of “setState on unmounted component” warning reactwg/react-18#82
Remove credential logging and add timeout cleanup on unmount.
Logging passwords and emails exposes sensitive data (privacy/compliance risk), and the uncleared setTimeout will attempt state updates after unmount, causing warnings and potential errors in React 18 Strict Mode.
Proposed fix
-import React, { useState } from "react";
+import React, { useEffect, useRef, useState } from "react";
@@
const [email, setEmail] = useState("");
const [password, setPassword] = useState("");
const [isLoading, setIsLoading] = useState(false);
+ const loginTimeoutRef = useRef(null);
@@
- setTimeout(() => {
- setIsLoading(false);
- console.log("Logging in with email:", email, " and password:", password);
- }, 2000);
+ loginTimeoutRef.current = window.setTimeout(() => {
+ setIsLoading(false);
+ console.log("Login submitted");
+ }, 2000);
};
+
+ useEffect(() => {
+ return () => {
+ if (loginTimeoutRef.current !== null) {
+ clearTimeout(loginTimeoutRef.current);
+ }
+ };
+ }, []);🤖 Prompt for AI Agents
In `@fy-project/src/components/Login/Login.jsx` around lines 20 - 28, Remove the
console.log that prints email and password inside handleSubmit to avoid
credential exposure, and ensure the simulated network setTimeout is cleaned up
on component unmount: store the timeout id (from setTimeout called in
handleSubmit) in a ref (e.g., loginTimeoutRef), clear that timeout in a
useEffect cleanup (or clear it when handleSubmit finishes) and guard
setIsLoading calls so they only run while mounted (clearing timeout prevents the
post-delay setIsLoading(false)). Update references to handleSubmit,
setIsLoading, and the setTimeout call accordingly and do not log email or
password anywhere.
1 participant
This PR introduces a few micro-UX and accessibility improvements:
aria-label="View shopping cart, 3 items". Its internal icon and badge are hidden from screen readers to prevent redundant announcements.:disabledstate for primary buttons has been added to both Login and Signup CSS files for visual consistency across the app.Visual verification was performed with Playwright, and a screenshot was captured showing the loading state.
PR created automatically by Jules for task 3796880956653555443 started by @BilalkhanIO
Summary by CodeRabbit
New Features
Documentation
Style