Skip to content

Solved Issue #764 - BUG: Back button in onboarding steps does not work #766

Closed
ritigya03 wants to merge 6 commits intoAOSSIE-Org:mainfrom
ritigya03:fix/onboarding-back-button
Closed

Solved Issue #764 - BUG: Back button in onboarding steps does not work #766
ritigya03 wants to merge 6 commits intoAOSSIE-Org:mainfrom
ritigya03:fix/onboarding-back-button

Conversation

@ritigya03
Copy link
Contributor

@ritigya03 ritigya03 commented Dec 13, 2025

Summary

This PR fixes an issue where the Back button in the onboarding flow was non-functional and did not navigate to the previous step.


Root cause

After reviewing the onboarding logic, the issue was caused by how the current step was resolved:

  • The active onboarding step was derived from step completion state (stepStatus.findIndex)
  • This works correctly when moving forward, but not when moving backward
  • Clicking Back only updated the completion state, not the actively rendered step
  • Additionally, some steps short-circuited rendering based on localStorage, which further prevented previous steps from appearing

As a result, Redux state was updated, but the UI did not re-render the expected previous step, making the Back button appear non-functional.


Solution

This PR ensures that backward navigation explicitly updates the active onboarding step instead of relying only on completion state.

  • Back navigation now correctly updates the step index
  • Step rendering no longer gets blocked by completion or localStorage guards
  • Forward navigation and existing UI remain unchanged

This results in predictable Back button behavior across all onboarding steps.


What this PR fixes

  • Makes Back button navigation functional and reliable
  • Ensures the correct previous step is rendered
  • Preserves the existing onboarding flow and UI

Short video

PictoPy.mp4

Related Issue

Closes #764

Team Name - EtherX

  • Ritigya Gupta
  • Heeral Mandolia
  • Sirjan Singh

Summary by CodeRabbit

  • New Features

    • Add an "editing" mode for onboarding so users can re-open and edit completed steps without them auto-completing.
  • Bug Fixes

    • Prevent steps from being marked complete on load while in editing mode.
  • Improvements

    • Progress labels and bar now use a safer display index for consistent progress.
    • Avatar/name/theme choices persist and are respected when navigating back and forth.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 13, 2025

📝 Walkthrough

Walkthrough

Threaded an optional currentStepDisplayIndex through onboarding steps, added an isEditing flag in the onboarding slice, and changed mount/complete logic to respect editing mode and persisted choices (localStorage). Progress/labels now use a computed displayIndex; back-navigation sets editing mode.

Changes

Cohort / File(s) Summary
Onboarding step UIs
frontend/src/components/OnboardingSteps/AvatarSelectionStep.tsx, frontend/src/components/OnboardingSteps/FolderSetupStep.tsx, frontend/src/components/OnboardingSteps/ThemeSelectionStep.tsx
Threaded optional currentStepDisplayIndexdisplayIndex fallback; added isEditing selector usage; mount effects and early-return rendering now check isEditing and persisted flags (localStorage); progress percent/labels use displayIndex; back handlers set setIsEditing(true); Theme step persists choice and navigates on completion.
Step orchestration
frontend/src/components/OnboardingSteps/OnboardingStep.tsx
Added optional currentStepDisplayIndex?: number; compute STEP_NAMES and clamp index to safeIndex; derive currentStepName and pass clamped stepIndex and currentStepDisplayIndex to children; render switch uses currentStepName.
Onboarding state slice
frontend/src/features/onboardingSlice.ts
Added isEditing: boolean to state and setIsEditing reducer/action; initialized isEditing to false and exported setIsEditing with other onboarding actions.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant UI as Onboarding Step Component
  participant Store as Redux (onboardingSlice)
  participant LS as localStorage
  participant Router as React Router

  rect rgb(240,248,255)
    UI->>LS: read persisted flag (e.g., name/avatar/themeChosen/folderChosen)
    UI->>Store: select isEditing
  end

  alt persisted && not isEditing
    UI->>Store: dispatch(markCompleted(stepIndex))
    Store-->>UI: state updated (completed)
    note right of UI: component may early-return (no render)
  else not persisted or isEditing
    UI->>UI: render step UI
    UI->>Store: (on Back) dispatch(setIsEditing(true))
    UI->>Router: navigate previousStep
    UI->>LS: (on Next) persist choice (localStorage.setItem)
    UI->>Store: dispatch(markCompleted(stepIndex)) when appropriate
  end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested labels

UI

Suggested reviewers

  • rahulharpal1603

Poem

🐰 I hopped through props and Redux trails tonight,
I set an edit flag, kept the progress right,
I check the burrow (localStorage) before I mark done,
Backwards I nudge — then hop onward with a bun 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title accurately describes the main objective: fixing the non-functional Back button in onboarding steps by addressing Issue #764.
Linked Issues check ✅ Passed The PR implementation directly addresses issue #764's requirements: Back button now navigates to previous steps via explicit step index updates, rendering guards removed, and UI re-renders correctly.
Out of Scope Changes check ✅ Passed All changes are scoped to fixing Back button navigation in onboarding steps; no unrelated modifications to other features or components detected.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🧹 Nitpick comments (3)
docs/backend/backend_python/openapi.json (1)

2204-2214: Minor schema clarity consideration: consider keeping explicit additionalProperties: true.

The metadata field's anyOf first option was simplified from { "additionalProperties": true, "type": "object" } to { "type": "object" }. While both are functionally equivalent (additionalProperties defaults to true in JSON Schema), explicitly stating it improves schema intent clarity for maintainers reading the spec. This is optional to address.

frontend/src/components/OnboardingSteps/OnboardingStep.tsx (1)

31-31: Non-standard comment style.

The comment prefix ✅ FIX: is non-standard. Consider using conventional comment prefixes like Note: or Fix: without emoji for better compatibility with IDEs and linters.

-  // ✅ FIX: derive stepName from stepIndex (single source of truth)
+  // Fix: derive stepName from stepIndex (single source of truth)
frontend/src/components/OnboardingSteps/ThemeSelectionStep.tsx (1)

45-45: Non-standard comment style.

The comment // ✅ THIS WAS MISSING uses emoji and all-caps, which is non-standard. Consider using conventional comment style for better tooling compatibility.

-    navigate(ROUTES.HOME); // ✅ THIS WAS MISSING
+    navigate(ROUTES.HOME); // Navigate to home after theme selection
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d07d817 and 103c245.

⛔ Files ignored due to path filters (1)
  • frontend/package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (5)
  • docs/backend/backend_python/openapi.json (1 hunks)
  • frontend/src/components/OnboardingSteps/AvatarSelectionStep.tsx (2 hunks)
  • frontend/src/components/OnboardingSteps/FolderSetupStep.tsx (3 hunks)
  • frontend/src/components/OnboardingSteps/OnboardingStep.tsx (2 hunks)
  • frontend/src/components/OnboardingSteps/ThemeSelectionStep.tsx (5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/components/OnboardingSteps/ThemeSelectionStep.tsx (2)
frontend/src/features/onboardingSlice.ts (2)
  • markCompleted (31-42)
  • previousStep (43-50)
frontend/src/constants/routes.ts (1)
  • ROUTES (1-12)
🔇 Additional comments (8)
docs/backend/backend_python/openapi.json (1)

1120-1127: Clear API documentation with sensible default.

The wrapping of the InputType reference in an allOf structure to add a title and default value improves the OpenAPI specification. This makes it explicit that "path" is the default input type, enhancing API clarity for client developers and documentation generation tools.

frontend/src/components/OnboardingSteps/FolderSetupStep.tsx (3)

17-17: Cleanup enables the back button fix.

Removing useEffect from imports is correct—the component no longer auto-completes on mount, which allows users to navigate back to this step even after completion.


54-56: Back button implementation is correct and properly wired to Redux state management.

The handleBack function dispatches previousStep(), which finds the last completed step, marks it as incomplete, and updates currentStepIndex to point to the first incomplete step. This ensures correct backward navigation through the onboarding flow.


1-149: No action required—step index handling is already consistent across all onboarding components.

All onboarding step components (AvatarSelectionStep, FolderSetupStep, ThemeSelectionStep) use the same indexing formula (stepIndex / totalSteps) * 100 for progress calculation and display Step {stepIndex} of {totalSteps} identically. The minor difference (AvatarSelectionStep calculates inline while FolderSetupStep and ThemeSelectionStep extract to a variable) does not affect consistency.

frontend/src/components/OnboardingSteps/AvatarSelectionStep.tsx (3)

1-1: LGTM! Clean removal of unused import.

The useEffect import was correctly removed since the auto-completion logic that relied on it has been eliminated as part of the back button fix.


44-50: LGTM! Removal of auto-completion guards fixes the back button issue.

The removal of the useEffect auto-completion logic and localStorage early-return guard (not visible in diff) correctly addresses the root cause described in the PR objectives. Previously, these guards prevented backward navigation by:

  • Auto-completing the step on mount
  • Hiding the component when localStorage contained data

The current implementation properly preserves forward navigation through handleNextClick while allowing the step to render normally during backward navigation.


58-66: Remove this review comment – the 0-based step indexing is intentional design, not a bug.

The onboarding flow uses 0-based step indexing throughout:

  • currentStepIndex starts at 0 in the Redux state (onboardingSlice.ts line 15)
  • All step components (AvatarSelectionStep, FolderSetupStep, ThemeSelectionStep) display steps using the same pattern: Step {stepIndex} of {totalSteps}
  • All progress calculations use (stepIndex / totalSteps) * 100 consistently
  • The markCompleted(stepIndex) action marks completion by index position, which is 0-based

This design is consistent across the entire onboarding system and there is no evidence of a refactoring that changed from 1-based to 0-based indexing. While displaying "Step 0" to users is unconventional (most UIs start at "Step 1"), this is an intentional architectural choice, not an accidental regression. Applying the suggested diff would break the system since all state management and array lookups rely on 0-based indexing.

If you believe the UX should display 1-based step numbers to users, that's a valid design improvement discussion – but it would require changing all step components and the state management logic together, not reverting to a non-existent previous state.

Likely an incorrect or invalid review comment.

frontend/src/components/OnboardingSteps/ThemeSelectionStep.tsx (1)

42-46: Navigation order is not a concern—the HOME route has no dependencies on onboarding state.

The dispatch(markCompleted) and navigate(ROUTES.HOME) order is correct. Redux state updates synchronously before the navigation occurs. However, this timing is actually irrelevant because the Home component and Layout do not read or depend on the onboarding state. There are no route guards, conditional renders, or component logic that checks onboarding completion status. The code works correctly as written.

interface OnboardingStepProps {
stepIndex: number;
stepName: string;
stepName: string; // kept as-is (not removed)
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Remove unused stepName prop.

The stepName prop is declared in the interface and accepted as a parameter but is never referenced in the component body. This is dead code that adds confusion.

Apply this diff to remove the unused prop:

 interface OnboardingStepProps {
   stepIndex: number;
-  stepName: string; // kept as-is (not removed)
 }
 export const OnboardingStep: React.FC<OnboardingStepProps> = ({
   stepIndex,
-  stepName, // still accepted, but not trusted
 }) => {

Also applies to: 24-24

🤖 Prompt for AI Agents
In frontend/src/components/OnboardingSteps/OnboardingStep.tsx around lines 13
and 24, the stepName prop is declared in the component's props interface and
accepted as a parameter but never used; remove stepName from the props interface
declaration and from the component's parameter list (and any related
destructuring) so only used props remain, then run TypeScript/ESLint to ensure
no remaining references and adjust any callsites if they explicitly passed
stepName.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing this out. The stepName prop is not currently used, but I’m keeping it intentionally as it may be needed in future iterations of the onboarding flow (e.g. for analytics, logging, or step-specific overrides). Retaining it avoids reintroducing the prop later and keeps the component interface flexible.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ritigya03, understood! Keeping the prop for future extensibility is a valid approach. I appreciate you explaining the reasoning—it makes sense to maintain interface stability if you anticipate needing it soon.

Comment on lines 43 to 45
localStorage.setItem('themeChosen', 'true');
dispatch(markCompleted(stepIndex));
navigate(ROUTES.HOME); // ✅ THIS WAS MISSING
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

rg -nP --type=ts --type=tsx -C 3 "themeChosen"

Repository: AOSSIE-Org/PictoPy

Length of output: 89


🏁 Script executed:

rg -nP -C 3 "themeChosen" --glob "*.ts" --glob "*.tsx"

Repository: AOSSIE-Org/PictoPy

Length of output: 701


Remove unused localStorage.setItem call.

Line 43 sets localStorage.setItem('themeChosen', 'true'), but this key is never read or consumed anywhere in the codebase. This is orphaned logic that should be removed since the PR already removed localStorage guards for back navigation handling.

🤖 Prompt for AI Agents
In frontend/src/components/OnboardingSteps/ThemeSelectionStep.tsx around lines
43 to 45, remove the unused localStorage.setItem('themeChosen', 'true') call (it
is orphaned and never read); keep the dispatch(markCompleted(stepIndex)) and
navigate(ROUTES.HOME) lines intact, and run a quick repo-wide search for
"themeChosen" to confirm no other references exist so no further cleanup is
needed.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

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/OnboardingStep.tsx (1)

44-47: Remove unreachable switch cases.

The UPDATE_STEP and SERVER_CHECK cases are now unreachable because currentStepName is derived from VISIBLE_STEPS, which only contains AVATAR_SELECTION_STEP, FOLDER_SETUP_STEP, and THEME_SELECTION_STEP. These cases are dead code that adds maintenance burden and confusion.

Apply this diff to remove the unreachable cases:

   const renderStepComponent = () => {
     switch (currentStepName) {
       case STEPS.AVATAR_SELECTION_STEP:
         return <AvatarSelectionStep {...sharedProps} />;
       case STEPS.FOLDER_SETUP_STEP:
         return <FolderSetupStep {...sharedProps} />;
       case STEPS.THEME_SELECTION_STEP:
         return <ThemeSelectionStep {...sharedProps} />;
-      case STEPS.UPDATE_STEP:
-        return <UpdateStep {...sharedProps} />;
-      case STEPS.SERVER_CHECK:
-        return <ServerCheck {...sharedProps} />;
       default:
         return <div></div>;
     }
   };
♻️ Duplicate comments (1)
frontend/src/components/OnboardingSteps/OnboardingStep.tsx (1)

13-13: Remove the unused stepName prop.

The stepName prop is declared in the interface and accepted as a parameter but is never referenced in the component body. Even with the "not trusted" comment, this remains dead code that adds confusion and maintenance burden.

Apply this diff to remove the unused prop:

 interface OnboardingStepProps {
   stepIndex: number;
-  stepName: string; 
 }
 export const OnboardingStep: React.FC<OnboardingStepProps> = ({
   stepIndex,
-  stepName, // still accepted, but not trusted
 }) => {

Also applies to: 24-24

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 103c245 and 5c14420.

📒 Files selected for processing (2)
  • frontend/src/components/OnboardingSteps/OnboardingStep.tsx (2 hunks)
  • frontend/src/components/OnboardingSteps/ThemeSelectionStep.tsx (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • frontend/src/components/OnboardingSteps/ThemeSelectionStep.tsx
🔇 Additional comments (1)
frontend/src/components/OnboardingSteps/OnboardingStep.tsx (1)

31-50: Core fix correctly addresses the Back button issue.

The explicit derivation of currentStepName from stepIndex establishes a single source of truth for step rendering, which directly addresses the root cause described in the PR objectives. By no longer relying on step completion state, backward navigation becomes reliable.

@rahulharpal1603
Copy link
Contributor

@ritigya03 Kindly resolve merge conflicts ASAP.

@github-actions github-actions bot added bug Something isn't working easy frontend good first issue Good for newcomers labels Dec 28, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (4)
frontend/src/components/OnboardingSteps/FolderSetupStep.tsx (1)

60-60: Progress and step numbering still use 0-based indexing.

This is the same issue flagged in previous reviews. The progress calculation stepIndex / totalSteps and step label Step {stepIndex} display 0-based values to users:

  • First step (stepIndex=0): shows "Step 0 of 3" with 0% progress
  • Second step (stepIndex=1): shows "Step 1 of 3" with 33% progress
  • Third step (stepIndex=2): shows "Step 2 of 3" with 66% progress

Users expect 1-based numbering and progress that reflects completion of the current step.

🔎 Apply the previously suggested fix
-  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: 68-68

frontend/src/components/OnboardingSteps/OnboardingStep.tsx (1)

34-36: Add lower bound check to prevent negative array index.

The safeIndex clamping only enforces an upper bound. If stepIndex is negative, VISIBLE_STEPS[safeIndex] returns undefined, causing the switch statement to render an empty fallback div instead of surfacing the invalid state.

🔎 Apply the previously suggested fix
   // FIX: derive stepName from stepIndex (single source of truth)
-  const safeIndex = Math.min(stepIndex, VISIBLE_STEPS.length - 1);
+  const safeIndex = Math.max(0, Math.min(stepIndex, VISIBLE_STEPS.length - 1));
   const currentStepName = VISIBLE_STEPS[safeIndex];
frontend/src/components/OnboardingSteps/ThemeSelectionStep.tsx (2)

45-45: Remove unused localStorage call.

This line sets localStorage.setItem('themeChosen', 'true'), but as flagged in previous reviews, this key is never read or consumed anywhere in the codebase. It should be removed since the PR already eliminated localStorage-based guards for navigation.

🔎 Proposed fix
   const handleNext = () => {
-    localStorage.setItem('themeChosen', 'true');
     dispatch(markCompleted(stepIndex));
     navigate(ROUTES.HOME);
   };

54-54: Progress and step numbering still use 0-based indexing.

These are the same issues flagged in previous reviews:

  1. Line 54: Progress calculation stepIndex / totalSteps produces incorrect percentages:

    • Theme step (stepIndex=2, totalSteps=3) shows 66% instead of 100%
  2. Line 62: Step display Step {stepIndex} shows 0-based numbers:

    • Theme step displays "Step 2 of 3" instead of "Step 3 of 3"

Users expect 1-based numbering and progress reflecting completion of the current step.

🔎 Apply the previously suggested fixes
-  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: 62-62

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5c14420 and 3421ba0.

📒 Files selected for processing (4)
  • frontend/src/components/OnboardingSteps/AvatarSelectionStep.tsx
  • frontend/src/components/OnboardingSteps/FolderSetupStep.tsx
  • frontend/src/components/OnboardingSteps/OnboardingStep.tsx
  • frontend/src/components/OnboardingSteps/ThemeSelectionStep.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
  • frontend/src/components/OnboardingSteps/AvatarSelectionStep.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/components/OnboardingSteps/ThemeSelectionStep.tsx (2)
frontend/src/features/onboardingSlice.ts (2)
  • markCompleted (31-42)
  • previousStep (43-50)
frontend/src/constants/routes.ts (1)
  • ROUTES (1-12)

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (5)
frontend/src/components/OnboardingSteps/ThemeSelectionStep.tsx (3)

45-45: Remove unused localStorage.setItem call.

Line 45 sets localStorage.setItem('themeChosen', 'true'), but this key is never read or consumed anywhere in the codebase. This is orphaned logic that should be removed.


54-54: Fix incorrect progress calculation.

The progress calculation stepIndex / totalSteps produces incorrect percentages. For the final step (Theme, stepIndex=2, totalSteps=3), this shows 67% instead of 100%.

Examples:

  • Avatar (stepIndex=0): 0/3 = 0%
  • Folder (stepIndex=1): 1/3 = 33%
  • Theme (stepIndex=2): 2/3 = 67% ❌ (should be 100%)
🔎 Proposed fix
-  const progressPercent = Math.round((stepIndex / totalSteps) * 100);
+  const progressPercent = Math.round(((stepIndex + 1) / totalSteps) * 100);

62-62: Fix step numbering for user display.

Line 62 displays Step {stepIndex} using the 0-based index directly. For the Theme step (stepIndex=2), this shows "Step 2 of 3" instead of the expected "Step 3 of 3". Users expect 1-based numbering (Step 1, Step 2, Step 3).

🔎 Proposed fix
-              Step {stepIndex} of {totalSteps}
+              Step {stepIndex + 1} of {totalSteps}
frontend/src/components/OnboardingSteps/FolderSetupStep.tsx (2)

60-60: Fix incorrect progress calculation.

The progress calculation stepIndex / totalSteps produces incorrect percentages. For this step when stepIndex=1, it shows 33% instead of 67%.

Examples:

  • Avatar (stepIndex=0): 0/3 = 0%
  • Folder (stepIndex=1): 1/3 = 33% ❌ (should be 67%)
  • Theme (stepIndex=2): 2/3 = 67% ❌ (should be 100%)
🔎 Proposed fix
-  const progressPercent = Math.round((stepIndex / totalSteps) * 100);
+  const progressPercent = Math.round(((stepIndex + 1) / totalSteps) * 100);

68-68: Fix step numbering for user display.

Line 68 displays Step {stepIndex} using the 0-based index directly. Users expect 1-based numbering (Step 1, Step 2, Step 3), but this will show "Step 0 of 3" for the first step.

🔎 Proposed fix
-              Step {stepIndex} of {totalSteps}
+              Step {stepIndex + 1} of {totalSteps}
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3421ba0 and 04bc221.

📒 Files selected for processing (4)
  • frontend/src/components/OnboardingSteps/AvatarSelectionStep.tsx
  • frontend/src/components/OnboardingSteps/FolderSetupStep.tsx
  • frontend/src/components/OnboardingSteps/OnboardingStep.tsx
  • frontend/src/components/OnboardingSteps/ThemeSelectionStep.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/src/components/OnboardingSteps/OnboardingStep.tsx (1)
frontend/src/constants/steps.ts (1)
  • STEPS (1-7)
frontend/src/components/OnboardingSteps/ThemeSelectionStep.tsx (4)
frontend/src/contexts/ThemeContext.tsx (1)
  • useTheme (66-73)
frontend/src/app/store.ts (1)
  • AppDispatch (24-24)
frontend/src/features/onboardingSlice.ts (2)
  • markCompleted (31-42)
  • previousStep (43-50)
frontend/src/constants/routes.ts (1)
  • ROUTES (1-12)

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
frontend/src/components/OnboardingSteps/ThemeSelectionStep.tsx (1)

56-59: Consider resetting isEditing to false on forward navigation.

Currently, isEditing is only set to true on back navigation but never reset to false on forward navigation. While this works correctly (initial state resets on page reload), explicitly resetting it in handleNext would make the state semantics clearer and prevent potential edge cases if Redux state becomes persisted.

🔎 Proposed change
 const handleNext = () => {
+   dispatch(setIsEditing(false));
   localStorage.setItem('themeChosen', 'true');
   dispatch(markCompleted(stepIndex));
 };

This same pattern could be applied to handleNextClick in AvatarSelectionStep and handleNext in FolderSetupStep for consistency.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 04bc221 and 639cda1.

📒 Files selected for processing (4)
  • frontend/src/components/OnboardingSteps/AvatarSelectionStep.tsx
  • frontend/src/components/OnboardingSteps/FolderSetupStep.tsx
  • frontend/src/components/OnboardingSteps/ThemeSelectionStep.tsx
  • frontend/src/features/onboardingSlice.ts
🧰 Additional context used
🧬 Code graph analysis (3)
frontend/src/components/OnboardingSteps/AvatarSelectionStep.tsx (2)
frontend/src/app/store.ts (1)
  • RootState (22-22)
frontend/src/features/onboardingSlice.ts (1)
  • markCompleted (36-47)
frontend/src/components/OnboardingSteps/FolderSetupStep.tsx (2)
frontend/src/app/store.ts (1)
  • RootState (22-22)
frontend/src/features/onboardingSlice.ts (3)
  • markCompleted (36-47)
  • setIsEditing (33-35)
  • previousStep (48-55)
frontend/src/components/OnboardingSteps/ThemeSelectionStep.tsx (2)
frontend/src/app/store.ts (1)
  • RootState (22-22)
frontend/src/features/onboardingSlice.ts (3)
  • markCompleted (36-47)
  • setIsEditing (33-35)
  • previousStep (48-55)
🔇 Additional comments (10)
frontend/src/features/onboardingSlice.ts (1)

12-12: Clean addition of isEditing state for back navigation support.

The new isEditing flag properly gates auto-completion and early-return logic in step components, enabling backward navigation without steps being skipped. The implementation follows Redux Toolkit conventions correctly.

Also applies to: 21-21, 33-35

frontend/src/components/OnboardingSteps/AvatarSelectionStep.tsx (3)

36-45: Good implementation of display index and editing state.

The displayIndex fallback pattern and isEditing selector integration are correctly implemented. Initializing local state from localStorage ensures form values are preserved when navigating back.


47-55: Proper gating of auto-completion and rendering with isEditing check.

The !isEditing condition correctly prevents auto-skipping when the user navigates backward, addressing the root cause of the back button bug.

Also applies to: 73-79


87-96: Step numbering and progress calculation are now correct.

The 1-based display (displayIndex + 1) and progress formula ((displayIndex + 1) / totalSteps) * 100 produce the expected user-facing values.

frontend/src/components/OnboardingSteps/FolderSetupStep.tsx (3)

70-73: Correct back navigation implementation.

Setting isEditing(true) before dispatching previousStep() ensures the previous step will render instead of auto-skipping based on localStorage state.


36-47: Consistent editing state integration.

The displayIndex fallback, isEditing selector, and guard conditions match the pattern established in AvatarSelectionStep, ensuring consistent behavior across all onboarding steps.

Also applies to: 75-77


79-87: Progress display correctly uses 1-based indexing.

The progress percentage and step label now use displayIndex + 1, showing user-friendly values like "Step 2 of 3" with 67% progress.

frontend/src/components/OnboardingSteps/ThemeSelectionStep.tsx (3)

39-50: Consistent implementation of editing state and guard logic.

The isEditing check in both the useEffect and early return correctly prevents the theme step from auto-completing when the user is navigating backward through the flow.


61-64: Back navigation correctly sets editing mode.

The handleBack function follows the same pattern as other steps, ensuring consistent backward navigation behavior.


70-78: Progress and step label correctly updated.

Using displayIndex + 1 ensures the final step shows "Step 3 of 3" with 100% progress, addressing the previous review concerns.

@rahulharpal1603
Copy link
Contributor

#892 is the new PR for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working easy frontend good first issue Good for newcomers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BUG: Back button in onboarding steps does not work

2 participants