fix: guard addFolderMutate call when no folder is selected in onboarding#1287
Conversation
WalkthroughThe PR adds a conditional guard to the folder-add mutation in the onboarding flow. The ChangesFolder Selection Validation Guard
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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: 2
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/FolderSetupStep.tsx (1)
38-42: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd missing dependencies to the useEffect dependency array.
The effect uses
dispatch,stepIndex, andisEditingbut the dependency array is empty. Per React hook best practices, all values referenced in the effect should be included in the dependency array to prevent stale closures.🔧 Add missing dependencies
useEffect(() => { if (localStorage.getItem('folderChosen') === 'true' && !isEditing) { dispatch(markCompleted(stepIndex)); } -}, []); +}, [dispatch, stepIndex, isEditing]);As per coding guidelines: Check for proper React hook dependencies and prevent unnecessary re-renders.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/OnboardingSteps/FolderSetupStep.tsx` around lines 38 - 42, The useEffect that reads localStorage and calls dispatch(markCompleted(stepIndex)) is missing dependencies and may capture stale values; update the effect's dependency array to include dispatch, stepIndex, and isEditing (i.e., useEffect(..., [dispatch, stepIndex, isEditing])) so the hook reacts to changes in those values while still preserving the existing guard (localStorage check and !isEditing); if you want it to run only on mount instead, explicitly justify and memoize any values (e.g., wrap markCompleted/dispatch usage) so ESLint rules are satisfied.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/components/OnboardingSteps/FolderSetupStep.tsx`:
- Around line 59-65: Add unit tests for the FolderSetupStep component to
exercise the handleNext guard: render FolderSetupStep, mock addFolderMutate and
the Redux dispatch (or spy on markCompleted), then simulate the user action that
triggers handleNext (e.g., click the Next button or call the exported handler).
Create two tests: (1) with folder undefined/empty assert addFolderMutate is not
called, dispatch(markCompleted(stepIndex)) is called, and
localStorage.setItem('folderChosen','true') was called; (2) with folder
populated assert addFolderMutate was called once with the folder,
dispatch(markCompleted(stepIndex)) is called, and
localStorage.setItem('folderChosen','true') was called. Use the component name
FolderSetupStep and the handler name handleNext to locate behavior and
reset/restore mocks and localStorage between tests.
- Line 60: The localStorage flag 'folderChosen' is being set unconditionally;
change the logic so you only call localStorage.setItem('folderChosen','true')
when a real folder is selected (e.g., inside the branch where the selected
folder variable is truthy in the Next/handleNext handler), and update all places
that read 'folderChosen' in the component's auto-complete logic to rely on that
true-only-when-selected behavior; alternatively, if you prefer to track step
completion instead, rename the key to 'folderStepCompleted' and update every
read/write of that key accordingly so semantics match.
---
Outside diff comments:
In `@frontend/src/components/OnboardingSteps/FolderSetupStep.tsx`:
- Around line 38-42: The useEffect that reads localStorage and calls
dispatch(markCompleted(stepIndex)) is missing dependencies and may capture stale
values; update the effect's dependency array to include dispatch, stepIndex, and
isEditing (i.e., useEffect(..., [dispatch, stepIndex, isEditing])) so the hook
reacts to changes in those values while still preserving the existing guard
(localStorage check and !isEditing); if you want it to run only on mount
instead, explicitly justify and memoize any values (e.g., wrap
markCompleted/dispatch usage) so ESLint rules are satisfied.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c8288016-71ad-4f2d-ba47-5463096ea4c4
📒 Files selected for processing (1)
frontend/src/components/OnboardingSteps/FolderSetupStep.tsx
2 participants
Addressed Issues:
Fixes #994
Screenshots/Recordings:
Before :
Change :
Could not test locally as the full backend setup was required to reach the onboarding screen. The fix is just a one line obvious change : addFolderMutate(folder) is only called when folder is a non-empty string, which is verifiably correct by code inspection.
AI Usage Disclosure:
I have used the following AI models and tools: Claude
Checklist
Summary by CodeRabbit