feat: Implement settings page functionality(#566)#606
Conversation
…s page, providing core account management features and improving the UI. Key features include: - Users can now update their display name and select a profile avatar from the account section. - The folder list is now restricted to an initial view of 6 items, with a 'View More' button for more.
…s page, providing core account management features and improving the UI. Key features include: - Users can now update their display name and select a profile avatar from the account section. - The folder list is now restricted to an initial view of 6 items, with a 'View More' button for more.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughReworks Settings into a tabbed UI (General / Account) with URL-hash sync. Adds new AccountSettingsCard (name + avatar persisted to localStorage and dispatched to Redux). FolderManagementCard now paginates folders (View More), uses stable ChangesSettings page tab navigation & wiring
Account settings component
Folder management pagination & per-item controls
Navbar link updates (routing)
Sequence DiagramsequenceDiagram
actor User
participant Navbar as Navbar
participant Settings as Settings.tsx
participant Tabs as TabBar
participant AccountCard as AccountSettingsCard
participant FolderCard as FolderManagementCard
participant Storage as localStorage
participant Redux as Redux (onboardingSlice)
User->>Navbar: Click avatar
Navbar->>Settings: Navigate to /settings#account
Settings->>Tabs: Render tab bar (General, Account)
User->>Tabs: Click "Account"
Tabs->>AccountCard: Render AccountSettingsCard
AccountCard->>Storage: Read saved name & avatar
Storage-->>AccountCard: Return values
User->>AccountCard: Edit name / select avatar
AccountCard->>AccountCard: Validate inputs
User->>AccountCard: Click "Save Changes"
AccountCard->>Storage: Persist name & avatar
AccountCard->>Redux: Dispatch setName & setAvatar
User->>Tabs: Click "General"
Tabs->>FolderCard: Render FolderManagementCard
FolderCard->>FolderCard: Show first N folders (slice)
User->>FolderCard: Click "View More"
FolderCard->>FolderCard: Increase visibleFoldersCount
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 (6)
frontend/src/pages/SettingsPage/Settings.tsx (1)
14-14: Consider using a string literal type for activeTab.The activeTab state uses plain strings which are prone to typos and lack IDE autocomplete support.
Add a type definition:
type TabType = 'general' | 'account'; const [activeTab, setActiveTab] = useState<TabType>('general');This provides type safety and better developer experience.
frontend/src/pages/SettingsPage/components/AccountSettingsCard.tsx (5)
1-1: Remove or relocate the implementation comment from the import line.The comment "No need for useEffect" on the import line is misplaced and adds confusion. If documenting a design decision, place it near the relevant logic or in the component docstring.
-import React, { useState } from 'react'; // No need for useEffect +import React, { useState } from 'react';
22-22: Remove the comment about removed code.Comments about removed code add no value and should be omitted. Git history tracks what was removed and why.
- // The redundant useEffect has been removed. -
35-48: Add user feedback after successful save.The save operation completes silently without confirming success to the user. This creates uncertainty about whether changes were persisted.
Consider adding a toast notification or temporary success message:
// After line 47, add: // Show success feedback (you'll need a toast/notification system) // toast.success('Settings saved successfully');Alternatively, add a local success state that shows a checkmark or message for a few seconds.
95-101: Improve image accessibility and error handling.The avatar images have generic alt text and no error handling for failed loads.
Apply this diff:
<img src={avatar} - alt="Avatar" + alt={`Avatar option ${avatars.indexOf(avatar) + 1}`} + onError={(e) => { + e.currentTarget.src = '/avatars/fallback.png'; // or hide the button + }} className={`h-24 w-24 rounded-full object-cover transition-all duration-300 ${ isSelected ? 'brightness-110' : '' }`} />This provides descriptive alt text for screen readers and graceful fallback for missing images.
110-116: Consider enabling Save button only when changes are detected.The button is currently enabled whenever an avatar is selected, even if no changes were made. This could lead to redundant saves.
Track initial values and compare:
const initialName = useRef(localStorage.getItem('name') || ''); const initialAvatar = useRef(localStorage.getItem('avatar') || ''); const hasChanges = name !== initialName.current || selectedAvatar !== initialAvatar.current; // Then use: disabled={!selectedAvatar || !hasChanges}This prevents unnecessary saves and provides clearer feedback about the form state.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/src/pages/SettingsPage/Settings.tsx(1 hunks)frontend/src/pages/SettingsPage/components/AccountSettingsCard.tsx(1 hunks)frontend/src/pages/SettingsPage/components/FolderManagementCard.tsx(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/src/pages/SettingsPage/components/FolderManagementCard.tsx (2)
frontend/src/components/ui/switch.tsx (1)
Switch(29-29)frontend/src/components/ui/progress.tsx (1)
Progress(37-37)
frontend/src/pages/SettingsPage/components/AccountSettingsCard.tsx (2)
frontend/src/features/onboardingSlice.ts (2)
setName(28-30)setAvatar(25-27)frontend/src/constants/avatars.ts (1)
avatars(1-10)
⏰ 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: Backend Tests
- GitHub Check: Tauri Build Check (macos-latest, --target aarch64-apple-darwin)
- GitHub Check: Tauri Build Check (ubuntu-22.04)
- GitHub Check: Tauri Build Check (windows-latest)
🔇 Additional comments (3)
frontend/src/pages/SettingsPage/components/FolderManagementCard.tsx (3)
32-36: LGTM!The pagination implementation correctly uses functional setState to avoid stale closure issues. Initial display of 6 folders with incremental loading of 5 more is a good UX balance.
46-50: LGTM!Excellent change to use
folder.folder_idas the key instead of array index. This ensures stable component identity during re-renders and list mutations.
139-147: LGTM!The View More button condition correctly shows when there are additional folders to display. The button styling and behavior are appropriate.
ShivaGupta-14
force-pushed
the
feat/settings-page
branch
from
bd927c2 to
1621717
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
frontend/src/pages/SettingsPage/components/AccountSettingsCard.tsx (4)
1-1: Remove development comment from production code.The inline comment "No need for useEffect" appears to be a development note and should be removed before merging.
Apply this diff:
-import React, { useState } from 'react'; // No need for useEffect +import React, { useState } from 'react';
23-23: Remove development comment.This comment about removed code should be deleted before merging.
Apply this diff:
- // The redundant useEffect has been removed. -
96-102: Improve alt text for accessibility.The alt text "Avatar" is generic and not helpful for screen reader users. Consider making it more descriptive.
Apply this diff:
<img src={avatar} - alt="Avatar" + alt={`Avatar option ${avatars.indexOf(avatar) + 1}`} className={`h-24 w-24 rounded-full object-cover transition-all duration-300 ${ isSelected ? 'brightness-110' : '' }`} />
111-117: Consider adding user feedback after save.The save operation completes silently without confirming success to the user. Consider adding a toast notification or temporary success message to improve UX.
Example using a toast library:
const handleSave = () => { if (!name.trim()) { setNameError(true); return; } setNameError(false); if (!selectedAvatar) return; try { localStorage.setItem('name', name); localStorage.setItem('avatar', selectedAvatar); dispatch(setName(name)); dispatch(setAvatar(selectedAvatar)); // Add success feedback toast.success('Settings saved successfully'); } catch (error) { console.error('Failed to save settings:', error); toast.error('Failed to save settings'); } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/pages/SettingsPage/Settings.tsx(1 hunks)frontend/src/pages/SettingsPage/components/AccountSettingsCard.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/pages/SettingsPage/Settings.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/pages/SettingsPage/components/AccountSettingsCard.tsx (2)
frontend/src/constants/avatars.ts (1)
avatars(1-10)frontend/src/features/onboardingSlice.ts (2)
setName(28-30)setAvatar(25-27)
⏰ 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: Backend Tests
- GitHub Check: Tauri Build Check (macos-latest, --target aarch64-apple-darwin)
- GitHub Check: Tauri Build Check (windows-latest)
- GitHub Check: Tauri Build Check (ubuntu-22.04)
🔇 Additional comments (2)
frontend/src/pages/SettingsPage/components/AccountSettingsCard.tsx (2)
17-20: Previous avatar validation concern has been addressed.The avatar value from localStorage is now properly validated against the avatars whitelist before use. This resolves the security concern from the previous review.
114-114: Good defensive programming.The disabled state on the save button properly prevents submission when no avatar is selected, making the early return check at line 43 a defensive measure that should never be reached in normal operation.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/pages/SettingsPage/components/AccountSettingsCard.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/pages/SettingsPage/components/AccountSettingsCard.tsx (2)
frontend/src/constants/avatars.ts (1)
avatars(1-10)frontend/src/features/onboardingSlice.ts (2)
setName(28-30)setAvatar(25-27)
🪛 Biome (2.1.2)
frontend/src/pages/SettingsPage/components/AccountSettingsCard.tsx
[error] 125-126: Illegal use of an export declaration not at the top level
move this declaration to the top level
(parse)
[error] 126-126: expected } but instead the file ends
the file ends here
(parse)
⏰ 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). (3)
- GitHub Check: Tauri Build Check (macos-latest, --target aarch64-apple-darwin)
- GitHub Check: Tauri Build Check (windows-latest)
- GitHub Check: Backend Tests
🔇 Additional comments (4)
frontend/src/pages/SettingsPage/components/AccountSettingsCard.tsx (4)
1-11: LGTM!Imports are clean and necessary. The removal of
useEffect(noted in the comment) aligns with the code cleanup mentioned in the AI summary.
17-20: LGTM! Avatar validation correctly implemented.The avatar value from localStorage is now properly validated against the
avatarsarray before use, with a safe fallback to an empty string. This addresses the previous security concern about arbitrary URLs.
25-34: LGTM!Event handlers are correctly implemented. The automatic clearing of
nameErrorwhen the user types provides good UX feedback.
54-123: LGTM!The component structure and JSX are well-organized:
- Input validation with clear visual feedback
- Avatar selection with appropriate visual states
- Save button correctly disabled when no avatar is selected
- Good accessibility with explicit button types
ShivaGupta-14
force-pushed
the
feat/settings-page
branch
from
89a300c to
e2c3e04
Compare
|
@rahulharpal1603 could you please check this PR? |
1 similar comment
|
@rahulharpal1603 could you please check this PR? |
|
@rahulharpal1603 could you please review this. |
|
Thanks for your contribution @ShivaGupta-14! |
2 participants
Solves Issue #566
This PR implements the user settings page, introducing account management features and folder list pagination as outlined in the issue.
Path
frontend/src/pages/SettingsPage.tsxfrontend/src/components/settings/AccountSettingsCard.tsxfrontend/src/pages/SettingsPage/components/FolderManagementCard.tsxApproach
Problem
Solution
localStorageto maintain state across sessions.Testing Done
localStorage.Summary
New Features
UX Improvements
Summary by CodeRabbit
New Features
Improvements