Fix useEffect dependency and cleanup issues in onboarding steps

[V4Vikaskumar]

What was fixed

• Fixed missing useEffect dependencies in onboarding steps
• Prevented state updates on unmounted components
• Aligned effects with React hook best practices

Files updated

• FolderSetupStep.tsx
• UpdateStep.tsx
• ServerCheck.tsx
• AvatarSelectionStep.tsx
• ThemeSelectionStep.tsx

Testing

• Frontend tests passing
• No lint warnings

Fixes #799

[coderabbitai]

Walkthrough

This PR refactors React hooks patterns across onboarding step components by fixing useEffect dependency arrays to include dispatch and stepIndex, adding mount guards to prevent stale closures, and introducing early returns for proper loading/error control flow handling.

Changes

Cohort / File(s)	Summary
Documentation docs/frontend/state-management.md	Added new "React Hooks Guidelines" section documenting best practices for useEffect dependencies, avoiding empty arrays when relying on props/dispatch/context/custom hooks, preventing stale closures, and ensuring ESLint rules pass.
Onboarding Step Components - Dependency Array Fixes frontend/src/components/OnboardingSteps/AvatarSelectionStep.tsx, FolderSetupStep.tsx, ThemeSelectionStep.tsx	Updated useEffect dependency arrays from [] to [dispatch, stepIndex] to ensure effects re-run when dependencies change; AvatarSelectionStep additionally refactored to compute hasName and hasAvatar from localStorage into local constants.
Onboarding Step Components - Advanced Control Flow frontend/src/components/OnboardingSteps/ServerCheck.tsx	Enhanced dependency array to include dispatch and stepIndex; added early returns in loading and error branches to prevent subsequent logic execution and ensure proper async handling.
Onboarding Step Components - Mount Guard frontend/src/components/OnboardingSteps/UpdateStep.tsx	Introduced isMount flag to guard async update check, added cleanup function to set flag on unmount, and expanded dependency array to [dispatch, stepIndex, checkForUpdates] to prevent state updates after component unmount.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• ServerCheck.tsx early returns and control flow logic require verification that error/loading paths correctly short-circuit
• UpdateStep.tsx mount guard pattern needs confirmation that async state updates are properly prevented and cleanup executes as intended
• Cross-component consistency: verify all dependency array changes align with the documented guidelines in the new documentation section

Possibly related PRs

• Fixed the Onboarding Page Setup #485: Directly modifies the same onboarding step component files with overlapping changes to useEffect patterns and component lifecycle logic.

Suggested labels

frontend

Poem

🐰 Hooks now dance with proper grace,
Dependencies find their place,
No stale closures left behind,
Mount guards keep the flow aligned,
Onboarding steps now purely refined! 🌟

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 (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main changes: fixing useEffect dependency arrays and adding cleanup mechanisms across onboarding step components to prevent stale closures and state updates on unmounted components.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

✨ 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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

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

43-58: Excellent fix: Early return prevents success logic after error.

The early return after showing the error dialog and scheduling exit ensures that the success-handling logic at lines 59-62 doesn't execute when there's an error. This prevents incorrect completion signals.

♻️ Duplicate comments (1)

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

33-37: Same navigation concern as FolderSetupStep.

The addition of stepIndex to the dependency array follows the same pattern as FolderSetupStep.tsx. The effect will re-run when users navigate between steps, potentially dispatching markCompleted multiple times. The localStorage check provides a guard, but verify this is the intended behavior.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d07d817 and 2d6756b.

⛔ Files ignored due to path filters (2)

• frontend/pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
• pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml

📒 Files selected for processing (6)

• docs/frontend/state-management.md
• frontend/src/components/OnboardingSteps/AvatarSelectionStep.tsx
• frontend/src/components/OnboardingSteps/FolderSetupStep.tsx
• frontend/src/components/OnboardingSteps/ServerCheck.tsx
• frontend/src/components/OnboardingSteps/ThemeSelectionStep.tsx
• frontend/src/components/OnboardingSteps/UpdateStep.tsx

🧰 Additional context used

🧬 Code graph analysis (3)

frontend/src/components/OnboardingSteps/ServerCheck.tsx (2)

frontend/src/features/loaderSlice.ts (1)

• hideLoader (21-24)

frontend/src/features/onboardingSlice.ts (1)

• markCompleted (31-42)

frontend/src/components/OnboardingSteps/UpdateStep.tsx (2)

frontend/src/features/loaderSlice.ts (2)

• showLoader (17-20)
• hideLoader (21-24)

frontend/src/features/onboardingSlice.ts (1)

• markCompleted (31-42)

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

frontend/src/features/onboardingSlice.ts (1)

• markCompleted (31-42)

🔇 Additional comments (6)

docs/frontend/state-management.md (1)

238-246: LGTM! Clear guidelines added.

The React Hooks Guidelines section provides practical guidance that aligns well with the code changes in this PR. Documenting these best practices helps maintain consistency across the codebase.

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

36-42: Good refactoring with constants; same navigation concern applies.

The refactoring to use hasName and hasAvatar constants improves readability. However, the same concern about stepIndex in the dependency array applies here as in the other onboarding steps—the effect will re-run when navigating between steps.

frontend/src/components/OnboardingSteps/UpdateStep.tsx (2)

26-44: The isMount guard pattern is acceptable for preventing updates after unmount.

The introduction of the isMount flag with cleanup is a valid pattern to prevent state updates on unmounted components. The early return on line 29-31 and the conditional dispatch on line 37 properly guard against stale updates.

---

25-45: No changes needed. The code in UpdateStep.tsx is correctly implemented. The checkForUpdates function is already memoized with useCallback and an empty dependency array in the useUpdater hook, making it a stable reference across renders. Including it in the effect dependency array will not cause infinite loops or performance issues.

Likely an incorrect or invalid review comment.

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

63-72: Dependency array updated consistently with other onboarding steps.

Adding dispatch and stepIndex to the dependency array aligns with the pattern applied across other onboarding components. The same navigation concern applies: if stepIndex changes during navigation, the effect will re-run.

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

33-37: Verify the actual navigation flow and stepIndex behavior: does stepIndex change when users navigate between steps? If stepIndex changes during navigation, the effect will re-run on each step change. While the localStorage guard prevents marking a step twice in succession, confirm that markCompleted is designed to handle multiple invocations safely during rapid navigation or step transitions. If idempotency is confirmed, consider adding a comment documenting this design choice; otherwise, consider using useEffectEvent or memoizing stepIndex to prevent unnecessary re-executions.

[coderabbitai]

🛠️ Refactor suggestion | 🟠 Major

Excellent fix: Early return prevents logic bugs during loading.

Adding the early return after dispatching the loader prevents the subsequent success-handling logic from executing while services are still loading. This is an essential fix that ensures proper control flow.

🤖 Prompt for AI Agents

In frontend/src/components/OnboardingSteps/ServerCheck.tsx around lines 39 to
42, ensure the early-return remains: when mainBackendLoading ||
syncMicroserviceLoading is true, dispatch showLoader('Waiting for servers to
start') and immediately return so that the subsequent success-handling logic
does not run while services are still loading; keep this control flow and remove
or guard any downstream success code that would otherwise execute during
loading.