FIX: #693 Registration navigation logic and add Sign Out button #694
Conversation
WalkthroughChanges span backend API schema updates to the OpenAPI specification, addition of react-router-dom dependency, refactoring of onboarding step components to adjust progress indexing and restore saved state from localStorage without auto-completion, and new sign-out functionality in the Settings page. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/OnboardingSteps/ThemeSelectionStep.tsx (1)
33-52: Theme step still auto-skips based on localStorage, blocking back-navigationBecause
useEffectmarks the step completed whenthemeChosenis inlocalStorage, and the component returnsnullin that case, this step effectively becomes unreachable once a theme has been chosen. That reintroduces the LS-driven auto-skip behavior you’re trying to avoid and prevents users from revisiting this step via Back.Consider removing the auto-skip logic and allowing the step to render regardless of
themeChosen:-import React, { useEffect } from 'react'; +import React from 'react'; @@ - useEffect(() => { - if (localStorage.getItem('themeChosen')) { - dispatch(markCompleted(stepIndex)); - } - }, []); @@ - if (localStorage.getItem('themeChosen')) { - return null; - }
🧹 Nitpick comments (5)
frontend/src/components/OnboardingSteps/ThemeSelectionStep.tsx (1)
54-69: Double-check stepIndex semantics to avoid “Step 0 of N” and 0% progress
progressPercentand the label currently usestepIndexdirectly. IfstepIndexis zero-based (0…totalSteps-1), the first step will render as “Step 0 of N” with 0% progress, which is usually not what users expect.If
stepIndexis indeed zero-based, consider:- const progressPercent = Math.round(((stepIndex) / totalSteps) * 100); + const progressPercent = Math.round(((stepIndex + 1) / totalSteps) * 100); @@ - Step {stepIndex} of {totalSteps} + Step {stepIndex + 1} of {totalSteps}If you’ve intentionally switched the container to 1-based indices, feel free to ignore this.
frontend/src/components/OnboardingSteps/FolderSetupStep.tsx (3)
52-62: Guard Next against empty folder selection
handleNextwill calladdFolderMutate(folder)even whenfolderis still an empty string, and the Next button is always enabled, so it’s easy to advance (and hit the API) without a valid path.Consider both guarding the handler and disabling the button until a folder is chosen:
const handleNext = () => { - localStorage.setItem('folderChosen', 'true'); - // FIX: Save the actual path so we can restore it later - localStorage.setItem('savedFolderPath', folder); - addFolderMutate(folder); - dispatch(markCompleted(stepIndex)); + if (!folder) { + return; + } + localStorage.setItem('folderChosen', 'true'); + // Save the actual path so we can restore it later + localStorage.setItem('savedFolderPath', folder); + addFolderMutate(folder); + dispatch(markCompleted(stepIndex)); }; @@ - <Button - onClick={handleNext} - className="cursor-pointer px-4 py-1 text-sm" + <Button + onClick={handleNext} + className="cursor-pointer px-4 py-1 text-sm" + disabled={!folder} >Also applies to: 139-152
52-55: Optionally clear persisted folder when the user removes itWhen the user removes the selected folder,
savedFolderPathremains inlocalStorage, so it will be restored on the next onboarding session even though the UI currently shows no folder.If that’s not desired, you could also clear the persisted value:
const handleRemoveFolder = () => { setFolder(''); + localStorage.removeItem('savedFolderPath'); };
68-80: Confirm stepIndex basis for folder step to avoid “Step 0 of N”
progressPercentand the label usestepIndexdirectly, which will show “Step 0 of N” and 0% progress ifstepIndexis zero-based.If that’s the case, you likely want:
- const progressPercent = Math.round(((stepIndex ) / totalSteps) * 100); + const progressPercent = Math.round(((stepIndex + 1) / totalSteps) * 100); @@ - Step {stepIndex } of {totalSteps} + Step {stepIndex + 1} of {totalSteps}If the parent now passes 1-based indices, the current code is fine, but it’d be good to double-check for consistency across all onboarding steps.
frontend/src/components/OnboardingSteps/AvatarSelectionStep.tsx (1)
71-81: Check avatar stepIndex usage to avoid “Step 0 of N” and 0%Here the header and progress both use
stepIndexdirectly. If your step indices are zero-based, the first step will display “Step 0 of N” with 0% progress.If that’s not intended, you can adjust like:
- Step {stepIndex} of {totalSteps} + Step {stepIndex + 1} of {totalSteps} @@ - <span>{Math.round(((stepIndex) / totalSteps) * 100)}%</span> + <span>{Math.round(((stepIndex + 1) / totalSteps) * 100)}%</span> @@ - style={{ width: `${((stepIndex) / totalSteps) * 100}%` }} + style={{ width: `${((stepIndex + 1) / totalSteps) * 100}%` }}As with the other steps, if the container was changed to pass 1-based indices, the current logic is fine—but worth verifying for consistency.
📜 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 (4)
frontend/package.json (1)
65-65: react-router-dom dependency matches new routing usageAdding
react-router-domis consistent with usinguseNavigatein the frontend. Please just confirm that this version is compatible with your existingreact-routerand React versions in your environment.frontend/src/pages/SettingsPage/Settings.tsx (1)
17-45: Sign-out flow correctly resets client stateClearing
localStorageand forcing a full reload to the welcome screen is consistent with the intent to fully reset Redux state and any background workers; the new destructive “Sign Out” button wiring looks good.docs/backend/backend_python/openapi.json (1)
1116-1127: OpenAPI tweaks to InputType and ImageInCluster.metadata look safeWrapping
InputTypein anallOfwith added description/default/title forinput_type, and simplifyingImageInCluster.metadatato an object-or-null union, both preserve the existing value space and keep clients compatible while improving schema metadata.Also applies to: 2205-2213
frontend/src/components/OnboardingSteps/AvatarSelectionStep.tsx (1)
36-47: Restoring avatar/name from localStorage keeps the step navigableLoading
nameandavatarinto local state instead of auto-skipping the component is a solid fix: it preserves prior input while still allowing users to revisit and edit this step when navigating back.
|
Hi @rahulharpal1603, |
|
Hi @rahulharpal1603 , I noticed this was closed without review. Since this fixes a breaking navigation bug (infinite loop on back button) and UI errors (Step 4 of 3) currently on the main branch, could you let me know if this was closed by mistake or if there's a specific reason? Thanks. |
2 participants
Fixes #693
Summary
This PR fixes issue #693 in the Onboarding and Settings UI. It resolves critical navigation bugs where users were trapped in infinite loops, the step counter jumped to non-existent steps (e.g., displaying "Step 4 of 3"), and there was no way to "Sign Out" to reset the application.What was happening
Navigation Logic Failures:
Infinite Loop: Onboarding components checked localStorage on mount and immediately auto-skipped. Clicking "Back" caused an instant bounce back to the current step.
Broken Step Counter: The step counter went from step 2 of 3, to step 4 of 3, instead of going from 1 of 3, to 2 of 3.
Settings Page:
No Exit Strategy: There was no "Sign Out" button. Users stuck in a crash loop (e.g., caused by selecting a bad folder) had no way to wipe their session and start over.
✅ Fix
Fixed Step Counter Logic: Corrected the state management to ensure the step counter increments sequentially (1 -> 2 -> 3) and and does not go to invalid steps like "Step 4".
Disabled Auto-Skip: Removed the useEffect hooks in onboarding steps that forced auto-completion, allowing true bidirectional navigation.
Added Data Retention: Implemented logic to restore form data (name, avatar, folderPath) from localStorage when navigating backward.
Added Sign Out: Implemented a "Sign Out" button in Settings.tsx using react-router-dom to wipe the session and hard-reset the app.
Result:
Correct Counting: Step counter now accurately reflects the user's progress (1 of 3 -> 2 of 3 -> 3 of 3).Working Navigation: Users can freely move "Back" and "Next" without getting trapped.
Emergency Reset: The Sign Out button successfully wipes the session, allowing users to recover from "stuck" states.
Data Persistence: Forms remain filled when navigating backward.
Changes
AvatarSelectionStep.tsx: Removed auto-skip, fixed step increment logic, added state restoration.FolderSetupStep.tsx: Removed auto-skip, fixed step increment logic, added folder persistence.
Settings.tsx: Added Sign Out button with useNavigate hook.
package.json: Added react-router-dom dependency.
Checklist
[x] I agree to follow this project's Code of Conduct[x] Navigation flows correctly (Back/Next) without loops
[x] Step counter correctly progresses (1 -> 2 -> 3) without skipping
[x] Sign Out button successfully resets the application
[x] No breaking changes introduced
Video.Project.2.mp4
Additional Notes
This PR introduces react-router-dom to handle navigation redirects. The "Sign Out" function uses a hard window reload (window.location.href) to ensure the Redux state and background scanners are completely severed from the previous session.Summary by CodeRabbit
New Features
Improvements
Dependencies
✏️ Tip: You can customize this high-level summary in your review settings.