FIX: #693 New registration and signout issues#701
Conversation
WalkthroughChanges modify the OpenAPI schema for image clustering endpoints, add react-router-dom dependency, refactor onboarding steps to fix progress tracking and state persistence using 0-based indexing, and add sign-out functionality to the Settings page with localStorage clearing. 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)
✅ 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: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
docs/backend/backend_python/openapi.json(1 hunks)frontend/package.json(1 hunks)frontend/src/components/OnboardingSteps/AvatarSelectionStep.tsx(2 hunks)frontend/src/components/OnboardingSteps/FolderSetupStep.tsx(3 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/pages/SettingsPage/Settings.tsx (1)
frontend/src/components/ui/button.tsx (1)
Button(59-59)
🔇 Additional comments (6)
docs/backend/backend_python/openapi.json (3)
1120-1128: OpenAPI schema refactor aligns with 3.1.0 best practices.The change from a direct
$refto anallOfcomposition with explicittitleand default metadata is a valid OpenAPI 3.1.0 pattern that improves schema clarity without changing the API contract. This allows tools and documentation generators to better understand the parameter intent.
2204-2213: Scope mismatch: backend API schema changes unrelated to PR objectives.The PR objectives describe frontend registration and sign-out flow fixes, yet this OpenAPI schema file contains backend changes. The changes themselves appear sound (tightening metadata validation by removing
additionalProperties), but their inclusion here suggests either unintended scope creep or incidental updates decoupled from the stated PR goals.Please clarify: Are these backend schema changes intentional and part of the same issue #693 fix, or were they accidentally included?
2204-2213: Verify that metadata schema constraint change does not break existing consumers.The removal of
additionalProperties: truefrom the metadata object now prevents unknown properties. If downstream consumers rely on storing additional metadata fields beyond the defined schema, this will break their integrations. This concern requires verification against actual usage patterns in the codebase to confirm whether any consumers depend on dynamic metadata properties.frontend/src/components/OnboardingSteps/FolderSetupStep.tsx (1)
34-37: LGTM: Proper state persistence for folder path.Loading the saved folder path from localStorage on mount and persisting it on Next enables proper state restoration during navigation, addressing the navigation issues mentioned in the PR objectives.
Also applies to: 57-57
frontend/src/components/OnboardingSteps/AvatarSelectionStep.tsx (1)
37-45: LGTM: Proper state restoration for name and avatar.Loading saved name and avatar from localStorage on mount enables proper state restoration during backward navigation, addressing the navigation issues mentioned in the PR objectives.
frontend/package.json (1)
65-65: No action required. React Router v7.10.1 is compatible with React 19.1.0 and is a stable, well-supported version with explicit React 19 compatibility.
| <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 UX issue: 0-based step indexing displays "Step 0 of 3".
The change from 1-based to 0-based indexing will display "Step 0 of 3" for the first step and show 0% progress, which is confusing for users. Step labels should be 1-based for display purposes even if internal indexing is 0-based.
Apply this diff to restore proper display indexing:
<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> <div
className="bg-primary h-full rounded-full transition-all duration-300"
- style={{ width: `${(stepIndex / totalSteps) * 100}%` }}
+ style={{ width: `${((stepIndex + 1) / totalSteps) * 100}%` }}
/>Also applies to: 72-72, 77-77
🤖 Prompt for AI Agents
In frontend/src/components/OnboardingSteps/AvatarSelectionStep.tsx around lines
70, 72 and 77, the UI is using a 0-based stepIndex directly which shows "Step 0
of 3" and 0% progress; update the display to use 1-based numbering by rendering
(stepIndex + 1) for the step label and adjust any progress calculation/display
to use (stepIndex + 1) or compute progress from (stepIndex + 1) over totalSteps
so the first step reads "Step 1 of N" and shows correct non-zero progress while
leaving internal indexing unchanged.
| return null; | ||
| } | ||
| const progressPercent = Math.round(((stepIndex + 1) / totalSteps) * 100); | ||
| const progressPercent = Math.round((stepIndex / totalSteps) * 100); |
There was a problem hiding this comment.
Critical UX issue: 0-based step indexing displays "Step 0 of 3".
The change from 1-based to 0-based indexing will display "Step 0 of 3" for the first step and show 0% progress, which is confusing for users. Step labels should be 1-based for display purposes even if internal indexing is 0-based.
Apply this diff to restore proper display indexing:
- const progressPercent = Math.round((stepIndex / totalSteps) * 100);
+ const progressPercent = Math.round(((stepIndex + 1) / totalSteps) * 100); <span>
- Step {stepIndex} of {totalSteps}
+ Step {stepIndex + 1} of {totalSteps}
</span>Also applies to: 74-74
| } | ||
|
|
||
| const progressPercent = Math.round(((stepIndex + 1) / totalSteps) * 100); | ||
| const progressPercent = Math.round((stepIndex / totalSteps) * 100); |
There was a problem hiding this comment.
Critical UX issue: 0-based step indexing displays "Step 0 of 3".
The change from 1-based to 0-based indexing will display "Step 0 of 3" for the first step and show 0% progress, which is confusing for users. Step labels should be 1-based for display purposes even if internal indexing is 0-based.
Apply this diff to restore proper display indexing:
- const progressPercent = Math.round((stepIndex / totalSteps) * 100);
+ const progressPercent = Math.round(((stepIndex + 1) / totalSteps) * 100); <span>
- Step {stepIndex} of {totalSteps}
+ Step {stepIndex + 1} of {totalSteps}
</span>Also applies to: 61-61
🤖 Prompt for AI Agents
In frontend/src/components/OnboardingSteps/ThemeSelectionStep.tsx around lines
54 and 61, the UI uses the 0-based stepIndex directly which produces "Step 0 of
N" and 0% progress on the first step; update the display and progress
calculation to use a 1-based value by adding 1 to stepIndex for user-facing text
and percent (e.g., use stepIndex+1 when computing progressPercent and when
rendering the "Step X of Y" label), and ensure values are clamped/rounded as
before so progressPercent remains an integer between 0 and 100.
| * Acts as an orchestrator for the settings sections | ||
| */ | ||
| const Settings: React.FC = () => { | ||
| const navigate = useNavigate(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove unused navigate variable.
The navigate hook is declared but never used in this component.
Apply this diff to remove the unused variable:
- const navigate = useNavigate();
-📝 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 navigate = useNavigate(); |
🤖 Prompt for AI Agents
In frontend/src/pages/SettingsPage/Settings.tsx around line 15, the hook
variable "const navigate = useNavigate();" is declared but never used; remove
that line and also remove the useNavigate import from the top of the file (or
the entire named import from react-router if it becomes unused) so there are no
unused variables/imports remaining.
| 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.
Inconsistent navigation approach: hard reload defeats React Router.
Using window.location.href = '/' triggers a full page reload, which negates the benefits of React Router's SPA navigation. This also conflicts with importing useNavigate.
If a hard reload is truly necessary to reset application state, remove the unused useNavigate import. Otherwise, use navigate('/') for proper SPA navigation and ensure state cleanup happens through proper React/Redux patterns.
Option 1: Use React Router navigation (recommended)
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 = '/';
+ navigate('/');
};Option 2: If hard reload is required, remove unused import
- import { useNavigate } from 'react-router-dom';
const Settings: React.FC = () => {
- const navigate = useNavigate();
-
const handleSignOut = () => {
localStorage.clear();
window.location.href = '/';
};📝 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 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 = '/'; | |
| }; | |
| const handleSignOut = () => { | |
| localStorage.clear(); | |
| navigate('/'); | |
| }; |
🤖 Prompt for AI Agents
In frontend/src/pages/SettingsPage/Settings.tsx around lines 17-22, the sign-out
handler clears localStorage then uses window.location.href='/' which forces a
hard reload and leaves the imported useNavigate unused; replace the hard reload
with React Router navigation by calling navigate('/') (after obtaining const
navigate = useNavigate()) and keep state cleanup via React/Redux flows, or if a
full reload is truly required remove the unused useNavigate import—choose one
approach and update the handler and imports accordingly.
|
Please resolve the merge conflicts before review. Your PR will only be reviewed by a maintainer after all conflicts have been resolved. 📺 Watch this video to understand why conflicts occur and how to resolve them: |
1 participant
Fixing issue #693
Summary
This pull request resolves issue #693 affecting both the Onboarding flow and the Settings UI. The update addresses several critical navigation and session-management defects, including infinite navigation loops, invalid step counter states (e.g., “Step 4 of 3”), and the absence of a session reset mechanism (“Sign Out”), which prevented users from recovering from corrupted or invalid configurations.
Root Cause Analysis
Infinite Loop in Onboarding:
Each onboarding step executed a useEffect on mount that attempted to auto-advance based on localStorage values. When users navigated backward, these effects immediately forced the UI to jump forward again, creating an infinite loop with no way to return to prior steps.
Invalid Step Counter State:
The step counter relied on inconsistent state transitions, causing jumps from 2 / 3 directly to invalid values such as 4 / 3. This occurred due to unguarded increments and incorrect step indexing.
No Exit Mechanism:
The Settings page did not expose a “Sign Out” or “Reset” option. This made it impossible for users to recover from misconfigurations (e.g., selecting an inaccessible folder), leaving them permanently trapped in a corrupted session state.
Fixes Implemented
✔ Corrected Step Counter Logic
Refactored state transitions to guarantee linear, bounded progression through onboarding steps (1 → 2 → 3). Guardrails prevent counter overflow into invalid states.
✔ Removed Auto-Skip Side Effects
Eliminated the useEffect hooks responsible for force-skipping onboarding steps. Users can now perform true bidirectional navigation without being redirected or blocked.
✔ State & Data Restoration
Implemented restoration of persisted form fields (name, avatar, folderPath) from localStorage. When navigating backward, previously entered data is rehydrated into the UI to provide a consistent onboarding experience.
✔ Added Full Sign-Out Support
Added a Sign Out button to Settings.tsx using react-router-dom.
Behavior:
Clears all persisted session data
Performs a controlled navigation back to the initial onboarding route
Restores the application to a clean state
Resulting Improvements
Accurate Step Progression: Step counter now reliably reflects the correct onboarding sequence.
Stability in Navigation: Users can move backward and forward without unintended redirects or loops.
Emergency Recovery: The new Sign Out control allows users to escape configuration lockouts or crash loops by fully resetting the app state.
Persistent User Input: All onboarding fields persist across navigations, improving user experience and preventing accidental data loss.
PictoPy.Shivansh.mp4
File-Level Changes
AvatarSelectionStep.tsx
Removed auto-advance logic
Corrected step increment/decrement mechanisms
Added localStorage-backed state restoration
FolderSetupStep.tsx
Removed auto-skip behavior
Stabilized step transition logic
Implemented folder path persistence
Settings.tsx
Added Sign Out button
Integrated useNavigate for controlled app reset
package.json
Added react-router-dom as a dependency
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.