Fixing issue #704#785
Conversation
…vatar-1-folder because i need signout feature for adding custom avatar functionality
|
|
WalkthroughThis pull request enhances the frontend onboarding and navigation workflows, migrates from 1-based to 0-based step indexing across onboarding components, adds persistent state restoration via localStorage, introduces custom avatar selection with file dialogs, integrates React Router navigation, and updates OpenAPI schema constraints. A new sign-out function is added to the Settings page. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
github-actions
Bot
added
UI
enhancement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (5)
frontend/src/components/OnboardingSteps/ThemeSelectionStep.tsx (1)
33-37: Add missing dependencies to useEffect.The effect uses
dispatchandstepIndexbut omits them from the dependency array, which can lead to stale closures and violates React Hooks rules.Apply this diff:
useEffect(() => { if (localStorage.getItem('themeChosen')) { dispatch(markCompleted(stepIndex)); } - }, []); + }, [dispatch, stepIndex]);frontend/src/components/OnboardingSteps/AvatarSelectionStep.tsx (2)
117-118: Remove non-English comment.The comment
// <-- yahan slice(0, 7)appears to be in Hindi. For consistency and maintainability, comments should be in English.- {avatars.slice(0, 7).map((avatar) => { - // <-- yahan slice(0, 7) + {avatars.slice(0, 7).map((avatar) => {
143-160: Consider showing a preview of the selected custom avatar.When a user selects a custom avatar, the button remains showing the "+" icon with a highlight ring. The user cannot see their selected image until they complete the step, which may cause confusion.
Consider displaying the selected custom avatar image:
<button type="button" onClick={handleCustomAvatarSelect} className={`bg-background relative inline-flex h-20 w-20 items-center justify-center rounded-full border transition-all duration-300 ${ selectedAvatar && !avatars.includes(selectedAvatar) ? 'border-primary ring-primary ring-offset-background ring-2 ring-offset-2' : 'border-muted' }`} > - <div className="flex flex-col items-center justify-center gap-1"> - <div className="bg-muted flex h-8 w-8 items-center justify-center rounded-full"> - <span className="text-lg leading-none">+</span> - </div> - <span className="text-muted-foreground text-[10px] font-medium"> - Custom Avatar - </span> - </div> + {selectedAvatar && !avatars.includes(selectedAvatar) ? ( + <img + src={selectedAvatar} + alt="Custom Avatar" + className="h-20 w-20 rounded-full object-cover" + /> + ) : ( + <div className="flex flex-col items-center justify-center gap-1"> + <div className="bg-muted flex h-8 w-8 items-center justify-center rounded-full"> + <span className="text-lg leading-none">+</span> + </div> + <span className="text-muted-foreground text-[10px] font-medium"> + Custom Avatar + </span> + </div> + )} </button>Note: Since
selectedAvatarstores an absolute file path from Tauri, verify that the<img src={...}>tag correctly renders local file paths in the Tauri webview context (may requireconvertFileSrcfrom@tauri-apps/api/core).frontend/src/pages/SettingsPage/Settings.tsx (1)
15-15: Remove unused navigate hook.The
navigatehook is declared but never used. Line 21 useswindow.location.hrefinstead.Apply this diff to remove the unused declaration:
- const navigate = useNavigate(); -Alternatively, refactor to use React Router navigation consistently (see next comment).
frontend/src/components/OnboardingSteps/FolderSetupStep.tsx (1)
63-64: Optional: Remove redundantfolderChosenflag.The
folderChosenflag is redundant now thatsavedFolderPathis persisted. A non-emptysavedFolderPathalready indicates that a folder has been chosen.- localStorage.setItem('folderChosen', 'true'); localStorage.setItem('savedFolderPath', folder);If other code checks for
folderChosen, update those locations to check forsavedFolderPathinstead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (9)
frontend/package-lock.jsonis excluded by!**/package-lock.jsonfrontend/public/avatars/avatar1.pngis excluded by!**/*.pngfrontend/public/avatars/avatar2.pngis excluded by!**/*.pngfrontend/public/avatars/avatar3.pngis excluded by!**/*.pngfrontend/public/avatars/avatar4.pngis excluded by!**/*.pngfrontend/public/avatars/avatar5.pngis excluded by!**/*.pngfrontend/public/avatars/avatar6.pngis excluded by!**/*.pngfrontend/public/avatars/avatar7.pngis excluded by!**/*.pngfrontend/public/avatars/avatar8.pngis excluded by!**/*.png
📒 Files selected for processing (7)
docs/backend/backend_python/openapi.json(1 hunks)frontend/package.json(1 hunks)frontend/src/components/Navigation/Navbar/Navbar.tsx(3 hunks)frontend/src/components/OnboardingSteps/AvatarSelectionStep.tsx(6 hunks)frontend/src/components/OnboardingSteps/FolderSetupStep.tsx(5 hunks)frontend/src/components/OnboardingSteps/ThemeSelectionStep.tsx(1 hunks)frontend/src/pages/SettingsPage/Settings.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/components/OnboardingSteps/AvatarSelectionStep.tsx (1)
frontend/src/constants/avatars.ts (1)
avatars(1-10)
🔇 Additional comments (9)
docs/backend/backend_python/openapi.json (2)
1120-1127: Valid OpenAPI enhancement: add descriptive metadata to InputType parameter.Wrapping the
InputTypereference inallOfto add a title and default value is a standard OpenAPI 3.1.0 pattern. The default"path"improves API documentation and client expectations.
2204-2214: Clarify intent of metadata schema change from constrained object to plain type.The metadata field in
ImageInClusternow accepts a plain object without explicit property constraints. This is more permissive, but reduces type safety and API contract clarity for clients consuming cluster metadata.Verify this relaxation is intentional and confirm that:
- Backend serialization always produces valid metadata.
- Frontend clients don't depend on strict schema validation for metadata keys.
- No downstream validation logic fails due to unexpected metadata shapes.
frontend/src/components/OnboardingSteps/AvatarSelectionStep.tsx (3)
21-26: LGTM!The import of
openfrom@tauri-apps/plugin-dialogand the updated interface withtotalStepsprop are appropriate for the new custom avatar functionality.
37-47: LGTM!State restoration from localStorage on mount is implemented correctly with proper null checks.
68-74: LGTM!The
handleNextClickhandler correctly persists state to both Redux and localStorage, ensuring data survives page refreshes and is available for the application state.frontend/src/components/Navigation/Navbar/Navbar.tsx (2)
19-22: LGTM! Correct handling of local file paths.The
avatarSrclogic properly handles custom local files viaconvertFileSrcand falls back appropriately to bundled avatars.
9-14: Remove unusedTooltipProviderimport.The
TooltipProvideris imported at line 12 but never used in this component. Each<Tooltip>component from@/components/ui/tooltipinternally wrapsTooltipPrimitive.Rootwith its own provider, so no manual provider is needed here. RemoveTooltipProviderfrom the import statement on lines 9-14.Likely an incorrect or invalid review comment.
frontend/src/components/OnboardingSteps/FolderSetupStep.tsx (2)
34-39: LGTM: Folder restoration from localStorage.The persistence and restoration logic correctly loads the saved folder path on mount, providing a good user experience if they navigate back to this step.
156-156: LGTM: Appropriate disabled button styling.The disabled state styling correctly provides visual feedback when the Next button is disabled, following good UX practices.
| "react-router": "^7.6.2", | ||
| "react-router-dom": "^7.10.1", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n frontend/package.json | sed -n '50,70p'Repository: AOSSIE-Org/PictoPy
Length of output: 851
🌐 Web query:
react-router-dom version 7.10.1 compatibility React 19
💡 Result:
Yes — react-router v7.10.1 is compatible with React 19. The v7 changelog and docs state v7 is a "bridge to React 19" and the v7.10.1 release (Dec 4, 2025) contains fixes and compatibility updates related to React 19. [1][2]
Sources:
- React Router changelog (v7.10.1 release notes, Dec 4, 2025). [1]
- React Router official docs / overview ("Bridge to React 19"). [2]
🌐 Web query:
react-router react-router-dom version alignment requirements
💡 Result:
Short answer — match major versions and follow React Router's packaging changes:
-
For React Router v6: install react-router-dom@6 (it re-exports the core react-router v6 APIs). Your project should have react-router-dom@6 (which depends on react-router v6) and react@18+ as required. [1]
-
For React Router v7+: the package structure changed — react-router is the main package and you should install react-router@7 (and remove react-router-dom); many third‑party libs declare peerDeps against a specific react-router major, so mixing v6 and v7 causes peer dependency errors. Always ensure dependent libs’ peerDependencies allow the react-router/react-router-dom major you plan to use. [1][2]
Practical checklist:
- Check package.json / peerDependencies of any library that integrates with React Router.
- Keep react-router and react-router-dom majors aligned (or switch to react-router only for v7+).
- If you see npm/yarn peer dependency errors, upgrade/downgrade the router package or the dependent library so their declared major versions match. [1][2]
Sources: React Router upgrade docs and peer-dependency discussion. [1] https://reactrouter.com/docs/upgrading/v6 [2] example peer-dependency conflict discussion.
Remove react-router-dom and use only react-router for v7.
React Router v7 restructured its packages — you should install only react-router@7 and remove react-router-dom from dependencies. The v7 architecture doesn't use react-router-dom as a separate package. Both versions are compatible with React 19, but the dependency structure is incorrect for v7.
🤖 Prompt for AI Agents
In frontend/package.json around lines 64–65, remove the "react-router-dom"
dependency and keep only "react-router" at the v7 specifier; then run your
package manager install to update lockfile. After removing the dependency,
update any code that imported from "react-router-dom" to import the equivalent
exports from "react-router" (adjust component names/usage per v7 API if needed),
run the app tests/build to catch import/name changes, and commit the updated
package.json and lockfile.
| {/* FaceSearch Dialog with Tooltip */} | ||
| <Tooltip> | ||
| <TooltipTrigger asChild> | ||
| <button | ||
| className="text-muted-foreground hover:bg-accent dark:hover:bg-accent/50 hover:text-foreground mx-1 cursor-pointer rounded-sm p-2" | ||
| title="Face Scanner" | ||
| aria-label="Face Scanner" | ||
| type="button" | ||
| > | ||
| <FaceSearchDialog /> | ||
| </button> | ||
| </TooltipTrigger> | ||
| </Tooltip> |
There was a problem hiding this comment.
Add missing TooltipContent for Face Scanner tooltip.
The Tooltip wrapper is incomplete—it has no TooltipContent, so nothing displays on hover. The PR objectives mention adding hover tooltips, but this implementation won't show any tooltip.
Apply this diff to complete the tooltip:
{/* FaceSearch Dialog with Tooltip */}
<Tooltip>
<TooltipTrigger asChild>
<button
className="text-muted-foreground hover:bg-accent dark:hover:bg-accent/50 hover:text-foreground mx-1 cursor-pointer rounded-sm p-2"
- title="Face Scanner"
aria-label="Face Scanner"
type="button"
>
<FaceSearchDialog />
</button>
</TooltipTrigger>
+ <TooltipContent side="bottom" sideOffset={8}>
+ Face Detection Scanner
+ </TooltipContent>
</Tooltip>Note: Removed redundant title attribute since the tooltip will provide the hover text.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {/* FaceSearch Dialog with Tooltip */} | |
| <Tooltip> | |
| <TooltipTrigger asChild> | |
| <button | |
| className="text-muted-foreground hover:bg-accent dark:hover:bg-accent/50 hover:text-foreground mx-1 cursor-pointer rounded-sm p-2" | |
| title="Face Scanner" | |
| aria-label="Face Scanner" | |
| type="button" | |
| > | |
| <FaceSearchDialog /> | |
| </button> | |
| </TooltipTrigger> | |
| </Tooltip> | |
| {/* FaceSearch Dialog with Tooltip */} | |
| <Tooltip> | |
| <TooltipTrigger asChild> | |
| <button | |
| className="text-muted-foreground hover:bg-accent dark:hover:bg-accent/50 hover:text-foreground mx-1 cursor-pointer rounded-sm p-2" | |
| aria-label="Face Scanner" | |
| type="button" | |
| > | |
| <FaceSearchDialog /> | |
| </button> | |
| </TooltipTrigger> | |
| <TooltipContent side="bottom" sideOffset={8}> | |
| Face Detection Scanner | |
| </TooltipContent> | |
| </Tooltip> |
🤖 Prompt for AI Agents
In frontend/src/components/Navigation/Navbar/Navbar.tsx around lines 73 to 85,
the Tooltip wrapper around the Face Scanner button is missing TooltipContent so
nothing appears on hover; remove the redundant title attribute from the button
and add a TooltipContent (with a TooltipArrow if your Tooltip API supports it)
containing the hover text (e.g., "Face Scanner") as a sibling to TooltipTrigger
inside the Tooltip so the tooltip displays on hover.
| {/* Profile Avatar with Tooltip – SAME HOVER STYLE */} | ||
| <Tooltip> | ||
| <TooltipTrigger asChild> | ||
| <button | ||
| type="button" | ||
| onClick={() => (window.location.href = '/settings')} | ||
| className="text-muted-foreground hover:bg-accent dark:hover:bg-accent/50 hover:text-foreground mx-1 cursor-pointer rounded-sm p-2" | ||
| aria-label="Profile" | ||
| > | ||
| <img | ||
| src={avatarSrc} | ||
| className="hover:ring-primary/50 h-8 w-8 rounded-full transition-all hover:ring-2" | ||
| alt="User avatar" | ||
| /> | ||
| </button> | ||
| </TooltipTrigger> | ||
| <TooltipContent side="bottom" sideOffset={8}> | ||
| Profile | ||
| </TooltipContent> | ||
| </Tooltip> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use React Router navigation instead of window.location.href.
Line 109 uses window.location.href = '/settings', which causes a full page reload and bypasses React Router. This is inconsistent with the new react-router-dom dependency added in this PR.
Refactor to use React Router's Link component or useNavigate hook:
Option 1 (preferred): Use Link component
+import { Link } from 'react-router-dom';
+
export function Navbar() {
// ... existing code ...
return (
// ...
{/* Profile Avatar with Tooltip – SAME HOVER STYLE */}
<Tooltip>
<TooltipTrigger asChild>
- <button
- type="button"
- onClick={() => (window.location.href = '/settings')}
+ <Link
+ to="/settings"
className="text-muted-foreground hover:bg-accent dark:hover:bg-accent/50 hover:text-foreground mx-1 cursor-pointer rounded-sm p-2"
aria-label="Profile"
>
<img
src={avatarSrc}
className="hover:ring-primary/50 h-8 w-8 rounded-full transition-all hover:ring-2"
alt="User avatar"
/>
- </button>
+ </Link>
</TooltipTrigger>Option 2: Use useNavigate hook
+import { useNavigate } from 'react-router-dom';
+
export function Navbar() {
+ const navigate = useNavigate();
// ... existing code ...
return (
// ...
- onClick={() => (window.location.href = '/settings')}
+ onClick={() => navigate('/settings')}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {/* Profile Avatar with Tooltip – SAME HOVER STYLE */} | |
| <Tooltip> | |
| <TooltipTrigger asChild> | |
| <button | |
| type="button" | |
| onClick={() => (window.location.href = '/settings')} | |
| className="text-muted-foreground hover:bg-accent dark:hover:bg-accent/50 hover:text-foreground mx-1 cursor-pointer rounded-sm p-2" | |
| aria-label="Profile" | |
| > | |
| <img | |
| src={avatarSrc} | |
| className="hover:ring-primary/50 h-8 w-8 rounded-full transition-all hover:ring-2" | |
| alt="User avatar" | |
| /> | |
| </button> | |
| </TooltipTrigger> | |
| <TooltipContent side="bottom" sideOffset={8}> | |
| Profile | |
| </TooltipContent> | |
| </Tooltip> | |
| import { Link } from 'react-router-dom'; | |
| // ... other imports ... | |
| export function Navbar() { | |
| // ... existing code ... | |
| {/* Profile Avatar with Tooltip – SAME HOVER STYLE */} | |
| <Tooltip> | |
| <TooltipTrigger asChild> | |
| <Link | |
| to="/settings" | |
| className="text-muted-foreground hover:bg-accent dark:hover:bg-accent/50 hover:text-foreground mx-1 cursor-pointer rounded-sm p-2" | |
| aria-label="Profile" | |
| > | |
| <img | |
| src={avatarSrc} | |
| className="hover:ring-primary/50 h-8 w-8 rounded-full transition-all hover:ring-2" | |
| alt="User avatar" | |
| /> | |
| </Link> | |
| </TooltipTrigger> | |
| <TooltipContent side="bottom" sideOffset={8}> | |
| Profile | |
| </TooltipContent> | |
| </Tooltip> |
| {/* Profile Avatar with Tooltip – SAME HOVER STYLE */} | |
| <Tooltip> | |
| <TooltipTrigger asChild> | |
| <button | |
| type="button" | |
| onClick={() => (window.location.href = '/settings')} | |
| className="text-muted-foreground hover:bg-accent dark:hover:bg-accent/50 hover:text-foreground mx-1 cursor-pointer rounded-sm p-2" | |
| aria-label="Profile" | |
| > | |
| <img | |
| src={avatarSrc} | |
| className="hover:ring-primary/50 h-8 w-8 rounded-full transition-all hover:ring-2" | |
| alt="User avatar" | |
| /> | |
| </button> | |
| </TooltipTrigger> | |
| <TooltipContent side="bottom" sideOffset={8}> | |
| Profile | |
| </TooltipContent> | |
| </Tooltip> | |
| import { useNavigate } from 'react-router-dom'; | |
| export function Navbar() { | |
| const navigate = useNavigate(); | |
| // ... existing code ... | |
| {/* Profile Avatar with Tooltip – SAME HOVER STYLE */} | |
| <Tooltip> | |
| <TooltipTrigger asChild> | |
| <button | |
| type="button" | |
| onClick={() => navigate('/settings')} | |
| className="text-muted-foreground hover:bg-accent dark:hover:bg-accent/50 hover:text-foreground mx-1 cursor-pointer rounded-sm p-2" | |
| aria-label="Profile" | |
| > | |
| <img | |
| src={avatarSrc} | |
| className="hover:ring-primary/50 h-8 w-8 rounded-full transition-all hover:ring-2" | |
| alt="User avatar" | |
| /> | |
| </button> | |
| </TooltipTrigger> | |
| <TooltipContent side="bottom" sideOffset={8}> | |
| Profile | |
| </TooltipContent> | |
| </Tooltip> |
🤖 Prompt for AI Agents
frontend/src/components/Navigation/Navbar/Navbar.tsx around lines 104 to 123:
the click handler currently uses window.location.href = '/settings' causing a
full reload; replace it with React Router navigation. Preferred fix: import Link
from react-router-dom and replace the button with a Link (to="/settings")
preserving className, aria-label and the img inside so tooltip Trigger asChild
still works. Alternate fix: import useNavigate, call const navigate =
useNavigate() at top of component and replace onClick with () =>
navigate('/settings') (remove window.location.href). Ensure you keep the
existing classes, aria-label, and TooltipTrigger asChild behavior.
| const handleCustomAvatarSelect = async () => { | ||
| const result = await open({ | ||
| multiple: false, | ||
| filters: [{ name: 'Images', extensions: ['png', 'jpg', 'jpeg'] }], | ||
| }); | ||
|
|
||
| if (typeof result === 'string') { | ||
| setLocalAvatar(result); // store absolute file path | ||
| } | ||
| }; |
There was a problem hiding this comment.
Add error handling for dialog failures.
The open() call can throw an exception if the dialog fails to open (e.g., permission issues, platform-specific errors). While user cancellation returns null (which is handled), unhandled exceptions will cause a runtime error.
const handleCustomAvatarSelect = async () => {
+ try {
const result = await open({
multiple: false,
filters: [{ name: 'Images', extensions: ['png', 'jpg', 'jpeg'] }],
});
if (typeof result === 'string') {
setLocalAvatar(result); // store absolute file path
}
+ } catch (error) {
+ console.error('Failed to open file dialog:', error);
+ }
};🤖 Prompt for AI Agents
In frontend/src/components/OnboardingSteps/AvatarSelectionStep.tsx around lines
57 to 66, the call to open() can throw and is currently unhandled; wrap the
await open(...) call in a try/catch, keep the existing check for user-cancel
(null) and only call setLocalAvatar(result) when result is a string, and in the
catch block log the error and surface a user-friendly message (e.g., toast or
modal) so the app doesn't crash on dialog failures.
| <div className="text-muted-foreground mb-1 flex justify-between text-xs"> | ||
| <span> | ||
| Step {stepIndex + 1} of {totalSteps} | ||
| Step {stepIndex} of {totalSteps} | ||
| </span> | ||
| <span>{Math.round(((stepIndex + 1) / totalSteps) * 100)}%</span> | ||
| <span>{Math.round((stepIndex / totalSteps) * 100)}%</span> | ||
| </div> | ||
| <div className="bg-muted mb-2 h-1.5 w-full rounded-full"> | ||
| <div | ||
| className="bg-primary h-full rounded-full transition-all duration-300" | ||
| style={{ width: `${((stepIndex + 1) / totalSteps) * 100}%` }} | ||
| style={{ width: `${(stepIndex / totalSteps) * 100}%` }} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for usages of AvatarSelectionStep
rg -n -B2 -A3 '<AvatarSelectionStep'Repository: AOSSIE-Org/PictoPy
Length of output: 682
🏁 Script executed:
# Find the component file to see its props definition
fd -type f -name "AvatarSelectionStep.tsx"Repository: AOSSIE-Org/PictoPy
Length of output: 232
🏁 Script executed:
# Look for step tracking logic in the parent component or context
rg -n "stepIndex\|totalSteps" frontend/src/components/OnboardingSteps/ -A2 -B2Repository: AOSSIE-Org/PictoPy
Length of output: 44
🏁 Script executed:
# Find the AvatarSelectionStep component file
find frontend -name "*AvatarSelectionStep*"Repository: AOSSIE-Org/PictoPy
Length of output: 125
🏁 Script executed:
# Look at OnboardingStep.tsx to understand sharedProps definition
cat -n frontend/src/components/OnboardingSteps/OnboardingStep.tsx | head -50Repository: AOSSIE-Org/PictoPy
Length of output: 1978
🏁 Script executed:
# Search for stepIndex initialization and how steps are tracked
rg -n "stepIndex|sharedProps" frontend/src/components/OnboardingSteps/OnboardingStep.tsx -B3 -A3Repository: AOSSIE-Org/PictoPy
Length of output: 1018
🏁 Script executed:
# Find where OnboardingStep is used to see how stepIndex is managed
rg -n '<OnboardingStep' -B3 -A3Repository: AOSSIE-Org/PictoPy
Length of output: 1066
🏁 Script executed:
# Search for step state/management logic
rg -n "stepIndex|currentStep" frontend/src/ -A2 -B2 | head -100Repository: AOSSIE-Org/PictoPy
Length of output: 7522
🏁 Script executed:
# Check if there's a parent component managing the step state
rg -n "useState.*step|setStep" frontend/src/ -B2 -A2 | head -100Repository: AOSSIE-Org/PictoPy
Length of output: 44
🏁 Script executed:
# Verify the actual code in AvatarSelectionStep.tsx at lines 80-89 to confirm
sed -n '80,89p' frontend/src/components/OnboardingSteps/AvatarSelectionStep.tsxRepository: AOSSIE-Org/PictoPy
Length of output: 559
🏁 Script executed:
# Check other step components to see if they have the same issue
rg -n 'Step \{stepIndex\}' frontend/src/components/OnboardingSteps/Repository: AOSSIE-Org/PictoPy
Length of output: 398
Add 1 to stepIndex when displaying progress to users.
stepIndex is 0-based (initialized to 0 in the Redux store), but displaying "Step 0 of 3" for the first step is confusing UX. Users expect 1-based display numbering. Additionally, the progress bar shows 0% on the first step, which is misleading.
Apply this fix to all three step components (AvatarSelectionStep, FolderSetupStep, and ThemeSelectionStep):
<span>
- Step {stepIndex} of {totalSteps}
+ Step {stepIndex + 1} of {totalSteps}
</span>
- <span>{Math.round((stepIndex / totalSteps) * 100)}%</span>
+ <span>{Math.round(((stepIndex + 1) / totalSteps) * 100)}%</span>And update the progress bar width accordingly:
- style={{ width: `${(stepIndex / totalSteps) * 100}%` }}
+ style={{ width: `${((stepIndex + 1) / totalSteps) * 100}%` }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div className="text-muted-foreground mb-1 flex justify-between text-xs"> | |
| <span> | |
| Step {stepIndex + 1} of {totalSteps} | |
| Step {stepIndex} of {totalSteps} | |
| </span> | |
| <span>{Math.round(((stepIndex + 1) / totalSteps) * 100)}%</span> | |
| <span>{Math.round((stepIndex / totalSteps) * 100)}%</span> | |
| </div> | |
| <div className="bg-muted mb-2 h-1.5 w-full rounded-full"> | |
| <div | |
| className="bg-primary h-full rounded-full transition-all duration-300" | |
| style={{ width: `${((stepIndex + 1) / totalSteps) * 100}%` }} | |
| style={{ width: `${(stepIndex / totalSteps) * 100}%` }} | |
| <div className="text-muted-foreground mb-1 flex justify-between text-xs"> | |
| <span> | |
| Step {stepIndex + 1} of {totalSteps} | |
| </span> | |
| <span>{Math.round(((stepIndex + 1) / totalSteps) * 100)}%</span> | |
| </div> | |
| <div className="bg-muted mb-2 h-1.5 w-full rounded-full"> | |
| <div | |
| className="bg-primary h-full rounded-full transition-all duration-300" | |
| style={{ width: `${((stepIndex + 1) / totalSteps) * 100}%` }} |
🤖 Prompt for AI Agents
In frontend/src/components/OnboardingSteps/AvatarSelectionStep.tsx around lines
80 to 89, the UI displays a 0-based stepIndex to users and uses it directly for
progress, producing "Step 0 of N" and 0% on the first step; update the displayed
step number to (stepIndex + 1) and compute progress percentage/width using
((stepIndex + 1) / totalSteps) * 100 (rounded for display) so the label shows
"Step 1 of N" and the progress bar reflects the correct portion; apply the same
change to FolderSetupStep and ThemeSelectionStep components.
|
|
||
| // Local state for folders | ||
| const [folder, setFolder] = useState<string>(''); | ||
| const [error, setError] = useState<string>(''); |
There was a problem hiding this comment.
Dead code: Error state is unreachable.
The error state and validation logic (lines 57-60, 62) can never execute because the Next button is disabled when !folder (line 155). The error message display (line 143) will never render.
Option 1 (Recommended): Remove the disabled attribute to allow error feedback:
- disabled={!folder}
- className="cursor-pointer px-4 py-1 text-sm disabled:cursor-not-allowed disabled:opacity-60"
+ className="cursor-pointer px-4 py-1 text-sm"Option 2: Remove the unreachable error handling code:
- const [error, setError] = useState<string>('');
// ... rest of component
const handleNext = () => {
- if (!folder) {
- setError('Please select at least one folder to continue.');
- return;
- }
-
- setError('');
localStorage.setItem('folderChosen', 'true');
// ... rest of handleNext
};
// ... later in JSX
- {error && <p className="px-3 pb-1 text-sm text-red-500">{error}</p>}Also applies to: 56-62, 143-143, 155-155
🤖 Prompt for AI Agents
frontend/src/components/OnboardingSteps/FolderSetupStep.tsx around lines 32,
56-62, 143, 155: the error state (useState at line 32) and its
validation/display logic (lines 56-62 and line 143) are unreachable because the
Next button is disabled when !folder (line 155), so either remove the disabled
attribute on the Next button to allow validation to run and show error messages,
or delete the error state, setError calls, and the error JSX block to remove
dead code; pick the recommended Option 1: enable the Next button (remove or
adjust the disabled prop) and ensure validation runs on click/submit so setError
and the error message can be reached and rendered.
| return null; | ||
| } | ||
| const progressPercent = Math.round(((stepIndex + 1) / totalSteps) * 100); | ||
| const progressPercent = Math.round((stepIndex / totalSteps) * 100); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, read the FolderSetupStep component to see the current code
cat -n frontend/src/components/OnboardingSteps/FolderSetupStep.tsx | sed -n '60,85p'Repository: AOSSIE-Org/PictoPy
Length of output: 1001
🏁 Script executed:
# Search for where FolderSetupStep is instantiated to understand how stepIndex is passed
rg -n -B2 -A5 'FolderSetupStep' --type=tsx --type=ts | head -100Repository: AOSSIE-Org/PictoPy
Length of output: 89
🏁 Script executed:
# Look for the parent component that manages stepIndex
rg -n 'stepIndex' frontend/src/components/OnboardingSteps/ --type=tsx --type=ts | head -50Repository: AOSSIE-Org/PictoPy
Length of output: 89
🏁 Script executed:
# Search for FolderSetupStep usage without specifying file type
rg -n 'FolderSetupStep' frontend/src --type=ts -A 3 -B 1Repository: AOSSIE-Org/PictoPy
Length of output: 2258
🏁 Script executed:
# Look for stepIndex in OnboardingSteps directory to find parent/state management
find frontend/src/components/OnboardingSteps -type f -name '*.tsx' -o -name '*.ts' | head -20Repository: AOSSIE-Org/PictoPy
Length of output: 474
🏁 Script executed:
# Find where stepIndex is incremented or initialized (likely in redux or state)
rg -n 'stepIndex' frontend/src --type=ts -B 2 -A 2 | head -80Repository: AOSSIE-Org/PictoPy
Length of output: 6205
🏁 Script executed:
# Check the initial state and nextStep/previousStep actions in onboardingSlice
rg -n 'initialState\|nextStep\|previousStep' frontend/src/features/onboardingSlice.ts -A 5 -B 1Repository: AOSSIE-Org/PictoPy
Length of output: 44
🏁 Script executed:
# Check the STEPS constants
cat -n frontend/src/constants/steps.tsRepository: AOSSIE-Org/PictoPy
Length of output: 315
🏁 Script executed:
# Check how OnboardingStep maps stepIndex to specific step components
cat -n frontend/src/components/OnboardingSteps/OnboardingStep.tsx | sed -n '1,45p'Repository: AOSSIE-Org/PictoPy
Length of output: 1759
🏁 Script executed:
# Get the complete onboardingSlice to understand state structure and step initialization
cat -n frontend/src/features/onboardingSlice.tsRepository: AOSSIE-Org/PictoPy
Length of output: 2289
🏁 Script executed:
# Check how the currentStepIndex is determined in InitialSteps
cat -n frontend/src/pages/InitialSteps/InitialSteps.tsxRepository: AOSSIE-Org/PictoPy
Length of output: 906
🏁 Script executed:
# Verify ThemeSelectionStep has the same issue
cat -n frontend/src/components/OnboardingSteps/ThemeSelectionStep.tsx | sed -n '50,65p'Repository: AOSSIE-Org/PictoPy
Length of output: 729
🏁 Script executed:
# Check if other step components have the same pattern
grep -n "progressPercent = Math.round" frontend/src/components/OnboardingSteps/*.tsxRepository: AOSSIE-Org/PictoPy
Length of output: 330
Fix progress calculation and step display: 0-based indexing should be displayed as 1-based to users.
The code uses 0-based step indexing (0, 1, 2 for 3 steps), but displays and calculates progress as if indices were user-facing, causing:
- "Step 0 of 3" instead of "Step 1 of 3" on first step
- Progress: 0%, 33%, 67% (final step never reaches 100%)
Apply this fix to both FolderSetupStep.tsx and ThemeSelectionStep.tsx:
- const progressPercent = Math.round((stepIndex / totalSteps) * 100);
+ const progressPercent = Math.round(((stepIndex + 1) / totalSteps) * 100);
// ...
- Step {stepIndex} of {totalSteps}
+ Step {stepIndex + 1} of {totalSteps}FolderSetupStep.tsx: lines 73, 81
ThemeSelectionStep.tsx: lines 54, 61
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/src/components/OnboardingSteps/FolderSetupStep.tsx lines 73 and 81
(and frontend/src/components/OnboardingSteps/ThemeSelectionStep.tsx lines 54 and
61), the code treats 0-based stepIndex as user-facing which shows "Step 0 of N"
and makes progress never reach 100%. Update the progressPercent calculation to
Math.round(((stepIndex + 1) / totalSteps) * 100) and display the step number as
stepIndex + 1 (e.g., "Step {stepIndex + 1} of {totalSteps}") so the UI is
1-based and the final step yields 100%.
| } | ||
|
|
||
| const progressPercent = Math.round(((stepIndex + 1) / totalSteps) * 100); | ||
| const progressPercent = Math.round((stepIndex / totalSteps) * 100); |
There was a problem hiding this comment.
Progress bar never reaches 100% on the final step.
With zero-based stepIndex, the formula (stepIndex / totalSteps) * 100 produces 0% on the first step and only 67% on the last step of a 3-step flow. Users expect to see 100% when completing the final step.
Apply this diff for completion-based progress:
- const progressPercent = Math.round((stepIndex / totalSteps) * 100);
+ const progressPercent = Math.round(((stepIndex + 1) / totalSteps) * 100);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const progressPercent = Math.round((stepIndex / totalSteps) * 100); | |
| const progressPercent = Math.round(((stepIndex + 1) / totalSteps) * 100); |
🤖 Prompt for AI Agents
In frontend/src/components/OnboardingSteps/ThemeSelectionStep.tsx around line
54, the progress calculation uses a zero-based stepIndex so (stepIndex /
totalSteps) never reaches 100% on the final step; change the formula to compute
completion-based progress by using (stepIndex + 1) as the numerator (i.e.,
((stepIndex + 1) / totalSteps) * 100) and round/clamp the result to the 0–100
range so the final step displays 100%.
| <div className="text-muted-foreground mb-1 flex justify-between text-xs"> | ||
| <span> | ||
| Step {stepIndex + 1} of {totalSteps} | ||
| Step {stepIndex} of {totalSteps} |
There was a problem hiding this comment.
Critical: User-facing step label displays zero-based index.
The step label shows "Step 0 of 3" for the first step instead of "Step 1 of 3", which is confusing for users who expect human-readable 1-based counting.
Apply this diff:
- Step {stepIndex} of {totalSteps}
+ Step {stepIndex + 1} of {totalSteps}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Step {stepIndex} of {totalSteps} | |
| Step {stepIndex + 1} of {totalSteps} |
🤖 Prompt for AI Agents
In frontend/src/components/OnboardingSteps/ThemeSelectionStep.tsx around line
61, the rendered label uses a zero-based stepIndex causing "Step 0 of X"; update
the display to use a 1-based index by rendering Step {stepIndex + 1} of
{totalSteps} (or wrap with Number(...) if stepIndex may be a string/undefined),
ensuring the UI shows human-readable counting.
| const handleSignOut = () => { | ||
| // 1. Wipe all local storage (Auth, Folder, Name) | ||
| localStorage.clear(); | ||
| // 2. Force a hard reload to reset state and go to Welcome screen | ||
| window.location.href = '/'; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Refactor sign-out to use React Router navigation and selective storage clearing.
Current issues:
localStorage.clear()removes ALL data, potentially breaking unrelated featureswindow.location.href = '/'bypasses React Router, causing a full page reload and defeating SPA benefits- Inconsistent with the imported
useNavigatehook
Apply this diff for a more graceful sign-out:
const handleSignOut = () => {
- // 1. Wipe all local storage (Auth, Folder, Name)
- localStorage.clear();
- // 2. Force a hard reload to reset state and go to Welcome screen
- window.location.href = '/';
+ // Clear only auth-related keys
+ localStorage.removeItem('authToken');
+ localStorage.removeItem('userName');
+ localStorage.removeItem('userAvatar');
+ localStorage.removeItem('selectedFolders');
+ // Navigate using React Router for SPA experience
+ navigate('/', { replace: true });
};This preserves unrelated localStorage data and maintains SPA navigation. If a full state reset is truly required, consider using navigate('/', { replace: true }) followed by a Redux store reset instead.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/src/pages/SettingsPage/Settings.tsx around lines 17 to 22, the
sign-out handler currently wipes all of localStorage and does a full-page
redirect; change it to only remove the auth-related keys (e.g., token/session,
user info, folder/name keys used by auth) instead of calling
localStorage.clear(), and use the imported useNavigate to perform SPA navigation
(navigate('/', { replace: true })) so the app stays within React Router; if you
need a full app state reset beyond clearing auth keys, invoke your Redux/Context
reset action after navigation rather than reloading the page.
|
Hey @paretar758-cloud and @rahulharpal1603 , Thank you for this well-documented PR! The improvements to onboarding validation, custom avatar support, and navigation tooltips are a great step toward a more polished and accessible user experience. I’ve reviewed the changes along with the feedback from CodeRabbit, and I noticed a few high-impact items that could help get this PR merge-ready:
As a GSoC '26 aspirant actively contributing to UX-focused open-source projects (with experience in React, Tauri, and frontend state management), I’d love to help address these issues. Would you be open to me:
Happy to collaborate! |
3 participants
Fixing issue #704
@rahulharpal1603 Sir, before proceeding further i want you to go through these videos to review my whole work. Sir you need to listen all these to review my work.
Part.-1.mp4
Part.-.2.mp4
Part.-.3.mp4
Part.-.4.mp4
Part.-.5.mp4
Title
Improve Onboarding Validation, Avatar Personalization, and Navigation UX
Overview
This PR introduces multiple improvements across the onboarding flow, user personalization, and UI accessibility in PictoPy. The changes address existing UX inconsistencies, prevent invalid application states, and improve feature discoverability, while also identifying architectural gaps required for long-term scalability.
Summary of Changes
Added a custom avatar selection option during user registration.
Enables users to select an image from the local file system using Tauri’s secure file dialog API.
Selected image is validated and integrated into the existing avatar selection workflow.
Improves user personalization and aligns with common desktop application UX standards.
Technical Notes:
File selection handled via Tauri backend bridge.
Avatar state management integrated into existing onboarding flow.
Compatible with current SQLite-based user metadata storage.
Added additional predefined avatar assets featuring characters from One Piece and Death Note.
Assets are optimized and integrated into the existing avatar grid without impacting layout responsiveness.
Provides greater expressive choice while maintaining consistency with current UI design.
Enforced minimum one folder selection before allowing the user to proceed in onboarding.
Added conditional validation logic to block progression when no folder is selected.
Displays a clear validation message prompting user action.
Technical Notes:
Validation implemented at the frontend state level (Redux-managed onboarding state).
Prevents incomplete gallery initialization and avoids downstream scan failures.
Fixed an incorrect onboarding navigation sequence where step numbering exceeded the defined flow (e.g., “step 4 of 3”).
Ensured proper step indexing and controlled transitions between onboarding stages.
Improves onboarding reliability and logical progression.
Added hover-based tooltips for:
Face Detection Scanner icon
Profile icon
Enhances feature discoverability without requiring user interaction.
Follows accessibility-friendly UI behavior and modern UX practices.
Technical Notes:
Implemented using lightweight tooltip components compatible with Tailwind/ShadCN.
Non-intrusive and does not affect existing click handlers.
Issues Identified and Reported (Non-Blocking)
6. Profile Section Architecture Gap
Reported the absence of a fully developed Profile section.
Highlighted the need for structured profile management (avatar updates, preferences, metadata).
Reported the need for a robust authentication-based login system.
Necessary for multi-user support, secure profile persistence, and future feature expansion.
Impact
Prevents invalid onboarding states.
Improves UI clarity and usability.
Enhances personalization and accessibility.
Establishes groundwork for future authentication and profile features.
Testing Performed
Verified onboarding flow with and without folder selection.
Tested avatar selection (default and custom upload).
Validated navigation step progression.
Confirmed tooltip visibility and interaction behavior.
Notes
All changes are backward-compatible and do not introduce breaking behavior.
No backend APIs were modified beyond existing integration boundaries.
Summary by CodeRabbit
Release Notes
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.