fix: resolve eslint react-hooks/set-state-in-effect violations - #22
Conversation
|
@Yuva-Deekshitha-N is attempting to deploy a commit to the Yuvraj Sarathe's projects Team on Vercel. A member of the Team first needs to authorize it. |
Reviewer's GuideRefactors three components/hooks to eliminate ESLint react-hooks/set-state-in-effect violations by moving setState logic out of useEffect into render-time derived state guarded by previous-value tracking, ensuring state updates are conditional and avoiding render loops. Sequence diagram for render-time derived state in AssessmentContentsequenceDiagram
participant React
participant AssessmentContent
participant sessionStorage
React->>AssessmentContent: render()
alt assessment_changed
AssessmentContent->>AssessmentContent: setShowCompletenessWarning(hasMissing)
end
alt assessment_and_githubData_changed
AssessmentContent->>sessionStorage: getItem(assessedCandidates)
AssessmentContent->>sessionStorage: setItem(assessedCandidates, JSON.stringify(stored))
AssessmentContent->>AssessmentContent: setSavedCandidates(stored)
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe changes derive assessment warning visibility from current data and dismissal state, persist assessed candidates immediately after generation, remove settings synchronization effects, and refactor loading-message rotation to use combined state. ChangesReact state and effect cleanup
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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
app/assessment/page.tsxOops! Something went wrong! :( ESLint: 9.39.1 TypeError: Converting circular structure to JSON app/settings/page.tsxOops! Something went wrong! :( ESLint: 9.39.1 TypeError: Converting circular structure to JSON hooks/useLoadingMessage.tsOops! Something went wrong! :( ESLint: 9.39.1 TypeError: Converting circular structure to JSON 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- The new pattern in
AssessmentContentperforms sessionStorage reads/writes and state updates during render, which are side effects and can behave poorly with React Strict Mode; consider moving this logic back into a guardeduseEffector a custom hook that satisfies the lint rule without doing work in the render phase. - The previous
useEffectforsavedCandidatesran on mount to hydrate existing candidates from sessionStorage even whenassessment/githubDatawere absent, whereas the new render-time block only runs when both are truthy; if this change is unintended, you may want a separate initialization path to preserve the original behaviour. - In
AssessmentContent, the reuse and ordering ofprevAssessmentRefbetween the completeness warning and savedCandidates logic means the second block will never treat assessment changes as a trigger in the same render; if assessment updates are expected to refresh saved candidates, consider using independent previous-value tracking or adjusting the control flow.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new pattern in `AssessmentContent` performs sessionStorage reads/writes and state updates during render, which are side effects and can behave poorly with React Strict Mode; consider moving this logic back into a guarded `useEffect` or a custom hook that satisfies the lint rule without doing work in the render phase.
- The previous `useEffect` for `savedCandidates` ran on mount to hydrate existing candidates from sessionStorage even when `assessment`/`githubData` were absent, whereas the new render-time block only runs when both are truthy; if this change is unintended, you may want a separate initialization path to preserve the original behaviour.
- In `AssessmentContent`, the reuse and ordering of `prevAssessmentRef` between the completeness warning and savedCandidates logic means the second block will never treat assessment changes as a trigger in the same render; if assessment updates are expected to refresh saved candidates, consider using independent previous-value tracking or adjusting the control flow.
## Individual Comments
### Comment 1
<location path="app/assessment/page.tsx" line_range="40-49" />
<code_context>
+ const prevAssessmentRef = useRef<AssessmentResult | null>(null);
+ const prevGithubDataRef = useRef<UserAssessmentData | null>(null);
+
+ // Derive showCompletenessWarning during render (not in an effect) to avoid
+ // ESLint react-hooks/set-state-in-effect.
+ if (assessment && assessment !== prevAssessmentRef.current) {
+ prevAssessmentRef.current = assessment;
+ const hasMissing = !assessment.summary || assessment.summary.length < 20 ||
+ !assessment.detailedReport || assessment.detailedReport.length < 200 ||
+ !assessment.repoAssessments || assessment.repoAssessments.length === 0 ||
+ !assessment.timeline || assessment.timeline.length === 0 ||
+ !assessment.swot.strengths || assessment.swot.strengths.length === 0;
+ if (hasMissing !== showCompletenessWarning) setShowCompletenessWarning(hasMissing);
+ }
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid calling setShowCompletenessWarning during render; move this logic into an effect or memoized derivation.
Setting state during render breaks React’s expectation that renders are pure and can cause subtle issues, especially with concurrent rendering. Instead, derive `hasMissing` from `assessment` in a `useMemo` and update `showCompletenessWarning` in a `useEffect`, or compute the warning condition directly from `assessment` without keeping separate state.
</issue_to_address>
### Comment 2
<location path="app/assessment/page.tsx" line_range="52-61" />
<code_context>
+ if (hasMissing !== showCompletenessWarning) setShowCompletenessWarning(hasMissing);
+ }
+
+ // Derive savedCandidates during render (not in an effect).
+ if (assessment && githubData && (assessment !== prevAssessmentRef.current || githubData !== prevGithubDataRef.current)) {
+ prevGithubDataRef.current = githubData;
+ const stored: ComparisonCandidate[] = JSON.parse(sessionStorage.getItem('assessedCandidates') || '[]');
+ const entry: ComparisonCandidate = { username: githubData.username, avatarUrl: githubData.avatarUrl, assessment };
+ const idx = stored.findIndex((c: ComparisonCandidate) => c.username === githubData.username);
+ if (idx >= 0) stored[idx] = entry;
+ else stored.push(entry);
+ try { sessionStorage.setItem('assessedCandidates', JSON.stringify(stored)); } catch {}
+ if (stored.length !== savedCandidates.length || stored.some((c, i) => c.username !== savedCandidates[i]?.username)) {
+ setSavedCandidates(stored);
+ }
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid running sessionStorage reads/writes and setSavedCandidates in the render path.
This block introduces side effects (sessionStorage access and `setSavedCandidates`) during render, violating React’s requirement that render be pure and risking repeated storage writes and state updates on every render where the condition passes. Move this logic into an effect keyed on `assessment` and `githubData`, and use that effect to keep storage and `savedCandidates` in sync instead of updating them during render.
</issue_to_address>
### Comment 3
<location path="hooks/useLoadingMessage.ts" line_range="78-83" />
<code_context>
- const [index, setIndex] = useState(0);
const shuffled = useMemo(() => shuffle(messages), [messages]);
+ const [index, setIndex] = useState(0);
+ const [prevShuffled, setPrevShuffled] = useState(shuffled);
- // Reset index when the message set changes.
- useEffect(() => {
- setIndex(0);
- }, [shuffled]);
+ // Reset index when the message set changes (derived state, no effect needed).
+ if (shuffled !== prevShuffled) {
+ setPrevShuffled(shuffled);
+ if (index !== 0) setIndex(0);
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** Resetting index via setState in the body of the hook introduces impure render logic.
The `if (shuffled !== prevShuffled)` guard avoids an immediate loop, but calling `setPrevShuffled` and `setIndex` directly in the hook body still breaks React’s requirement that components/hooks be pure during render. This can conflict with concurrent rendering and lead to subtle bugs. Please move the reset logic into a `useEffect` keyed on `shuffled` so state updates happen outside the render phase while still resetting `index` appropriately.
</issue_to_address>
### Comment 4
<location path="app/settings/page.tsx" line_range="13-18" />
<code_context>
const router = useRouter();
const { settings, updateSettings } = useStore();
const [localSettings, setLocalSettings] = useState(settings);
+ const [prevSettings, setPrevSettings] = useState(settings);
- useEffect(() => {
+ // Sync localSettings when the store settings object is replaced externally.
+ if (settings !== prevSettings) {
+ setPrevSettings(settings);
setLocalSettings(settings);
- }, [settings]);
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** Synchronizing localSettings from settings via setState in render should be moved back into an effect.
This `if (settings !== prevSettings) { ... }` block runs during render and triggers state updates, which is incompatible with React’s concurrent rendering and can cause subtle, hard-to-debug issues. The sync logic should live in a `useEffect` keyed on `settings` so renders remain pure while `localSettings` stays aligned with the store when `settings` changes.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Yuvraj-Sarathe
added
bug
|
@Yuva-Deekshitha-N resolve these suggestions and comments from Sourcery and you are ready to go! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@app/assessment/page.tsx`:
- Around line 50-56: Update the sessionStorage handling in the assessment effect
around the stored candidate array: move storage reads and JSON parsing inside
the existing try/catch, validate that the parsed value is an array of valid
ComparisonCandidate entries before using it, and fall back to [] for unavailable
storage, malformed JSON, or invalid shapes. Preserve the existing
replacement/push behavior and ensure setSavedCandidates still receives the safe
fallback or validated array.
- Around line 40-45: The assessment page’s local hasMissing predicate duplicates
incomplete and inconsistent criteria. Reuse the canonical completeness validator
from lib/ai.ts in the assessment flow, passing the full assessment and using its
result for setShowCompletenessWarning; remove the partial field checks so the UI
applies all eight criteria and the required issue threshold.
In `@app/settings/page.tsx`:
- Around line 14-17: Update the settings synchronization in the page component
so `localSettings` is reconciled during render when the `settings` object
identity changes, tracking the previous `settings` value to avoid unnecessary
updates. Remove the `useEffect` import and effect once unused, while preserving
synchronization for externally replaced settings.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0f7af396-f5d1-4bb0-8672-20cd08c0ff48
📒 Files selected for processing (3)
app/assessment/page.tsxapp/settings/page.tsxhooks/useLoadingMessage.ts
| const hasMissing = !assessment.summary || assessment.summary.length < 20 || | ||
| !assessment.detailedReport || assessment.detailedReport.length < 200 || | ||
| !assessment.repoAssessments || assessment.repoAssessments.length === 0 || | ||
| !assessment.timeline || assessment.timeline.length === 0 || | ||
| !assessment.swot.strengths || assessment.swot.strengths.length === 0; | ||
| setShowCompletenessWarning(hasMissing); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reuse the canonical completeness criteria.
This predicate checks only five fields and warns when any one is missing, while lib/ai.ts Lines 264-279 checks eight fields and considers the assessment incomplete only after at least three issues. The UI can therefore show false warnings or miss incomplete assessments. Share the canonical validator instead of duplicating a partial predicate.
🤖 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 `@app/assessment/page.tsx` around lines 40 - 45, The assessment page’s local
hasMissing predicate duplicates incomplete and inconsistent criteria. Reuse the
canonical completeness validator from lib/ai.ts in the assessment flow, passing
the full assessment and using its result for setShowCompletenessWarning; remove
the partial field checks so the UI applies all eight criteria and the required
issue threshold.
| const stored: ComparisonCandidate[] = JSON.parse(sessionStorage.getItem('assessedCandidates') || '[]'); | ||
| const entry: ComparisonCandidate = { username: githubData.username, avatarUrl: githubData.avatarUrl, assessment }; | ||
| const idx = stored.findIndex((c: ComparisonCandidate) => c.username === githubData.username); | ||
| if (idx >= 0) stored[idx] = entry; | ||
| else stored.push(entry); | ||
| try { sessionStorage.setItem('assessedCandidates', JSON.stringify(stored)); } catch {} | ||
| setSavedCandidates(stored); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard sessionStorage reads and validate the stored shape.
getItem and JSON.parse are outside the try block, and non-array or malformed data is used as ComparisonCandidate[] without validation. A corrupted/stale value or unavailable storage can abort this effect before savedCandidates is updated. Catch read/parse failures and accept only validated candidate arrays, falling back to [].
🤖 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 `@app/assessment/page.tsx` around lines 50 - 56, Update the sessionStorage
handling in the assessment effect around the stored candidate array: move
storage reads and JSON parsing inside the existing try/catch, validate that the
parsed value is an array of valid ComparisonCandidate entries before using it,
and fall back to [] for unavailable storage, malformed JSON, or invalid shapes.
Preserve the existing replacement/push behavior and ensure setSavedCandidates
still receives the safe fallback or validated array.
| // Sync localSettings when the store settings object is replaced externally. | ||
| useEffect(() => { | ||
| setLocalSettings(settings); | ||
| }, [settings]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repo files matching app/settings/page.tsx:"
fd -a 'page\.tsx$' . | sed 's#^\./##' | grep 'app/settings/page.tsx' || true
if [ -f app/settings/page.tsx ]; then
echo
echo "app/settings/page.tsx outline:"
ast-grep outline app/settings/page.tsx || true
echo
echo "app/settings/page.tsx relevant content:"
nl -ba app/settings/page.tsx | sed -n '1,120p'
fi
echo
echo "Search for eslint config/plugins/rules and set-state-in-effect usages:"
rg -n "react-hooks|eslint-plugin-react-hooks|set-state-in-effect|no-set-state-in-effect|eslintConfig|eslint" -S --glob '!node_modules' --glob '!dist' --glob '!build' . | sed -n '1,160p'Repository: Yuvraj-Sarathe/GitDeep
Length of output: 424
🌐 Web query:
React hooks eslint set-state-in-effect lint rules state in effect synchronous call allowed?
💡 Result:
Yes, it is generally allowed to call a state setter function (setState) within a useEffect hook [1][2], provided it is done correctly to avoid issues like infinite loops [1]. However, there is a specific ESLint rule, react-hooks/set-state-in-effect, which validates against calling setState synchronously inside an effect [3]. Key considerations regarding this practice: 1. Performance Concerns: The react-hooks/set-state-in-effect rule warns against synchronous setState calls because they can trigger unnecessary re-renders, degrade performance, and cause visual jank [3]. When you update state in an effect, React must re-render the component, re-apply changes, and re-run effects, creating an extra render pass [3]. 2. Alternatives: If the state update is based on existing props or state, it is often better to calculate that value during the render phase rather than using useEffect to update it later [3]. This simplifies the component and avoids the extra render cycle [3]. 3. Avoiding Infinite Loops: When you do use setState in an effect, ensure you have a proper dependency array [1]. If you update state based on its previous value, use the functional update form (e.g., setCounter(prev => prev + 1)) to avoid needing the state variable itself in the dependency array [4][1][2]. 4. Dependency Stability: You do not need to include the setState setter function itself in the useEffect dependency array, as React guarantees its identity is stable across renders [5][2]. 5. Linting Rules: - react-hooks/set-state-in-effect: Focuses on the performance impact of synchronous updates within effects [3][6]. - react-hooks/exhaustive-deps: Ensures that all reactive values used inside the effect are included in the dependency array to prevent stale closures [7][8]. In summary, while technically possible, you should aim to minimize synchronous state updates within effects by deriving data during render whenever possible [3]. If an update is necessary, ensure it is carefully guarded or triggered by appropriate dependencies to prevent infinite re-render loops [1].
Citations:
- 1: https://stackoverflow.com/questions/53715465/can-i-set-state-inside-a-useeffect-hook
- 2: https://atomizedobjects.com/blog/react/how-to-use-setstate-from-within-useeffect-with-dependencies/
- 3: https://react.dev/reference/eslint-plugin-react-hooks/lints/set-state-in-effect
- 4: [react-hooks/exhaustive-deps] eslint --fix breaks the code react/react#15204
- 5: https://legacy.reactjs.org/docs/hooks-reference.html
- 6: https://www.npmjs.com/package/eslint-plugin-react-hooks
- 7: https://react.dev/reference/eslint-plugin-react-hooks/lints/exhaustive-deps
- 8: https://blog.logrocket.com/understanding-react-exhaustive-deps-linting-warning/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "app/settings/page.tsx content:"
awk '{printf "%6d %s\n", NR, $0}' app/settings/page.tsx
echo
echo "All eslint config/package mentions:"
for f in package.json .eslintrc .eslintrc.js .eslintrc.cjs eslint.config.js eslint.config.mjs eslint.config.cjs; do
[ -f "$f" ] && { echo "--- $f"; awk '{printf "%6d %s\n", NR, $0}' "$f"; }
done
echo
echo "Search react-hooks rules:"
rg -n "react-hooks|set-state-in-effect|setState|setLocalSettings|settings" -S --glob '!node_modules' --glob '!dist' --glob '!build' . | sed -n '1,220p'Repository: Yuvraj-Sarathe/GitDeep
Length of output: 50379
Move the localSettings sync out of useEffect.
setLocalSettings(settings) inside useEffect still triggers the react-hooks/set-state-in-effect lint from eslint-config-next; the comment on the effect does not suppress the rule. Track the previous settings value during render and update localSettings there, then remove useEffect if it is no longer needed.
🤖 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 `@app/settings/page.tsx` around lines 14 - 17, Update the settings
synchronization in the page component so `localSettings` is reconciled during
render when the `settings` object identity changes, tracking the previous
`settings` value to avoid unnecessary updates. Remove the `useEffect` import and
effect once unused, while preserving synchronization for externally replaced
settings.
2 participants
🔗 Related Issue
Closes #11
📝 Description of Changes
ESLint's react-hooks/set-state-in-effect rule was failing on every PR because three files had unconditional setState calls inside useEffect. This is a repo-wide CI blocker, not introduced by any contributor.
Root cause: Calling setState unconditionally inside useEffect can trigger cascading renders (effect fires → setState → re-render → effect fires again). ESLint flags this to prevent subtle render loops.
Fix applied: Replaced all offending patterns with React's idiomatic "derived state during render" approach — track the previous dependency value in a second useState, compare during render, and call setState conditionally in the render body (not inside an effect). This is the pattern recommended in the React docs for syncing state to a changing prop/dependency.
Files fixed:
🏷️ Proposed Labels
📂 Core Files Changed
📸 Verification & Screenshots
UI/UX: No visual changes. All UI behaviour is identical — loading messages still cycle, settings still sync, completeness warning still appears/dismisses correctly.
CI/CD: npm run lint — exit code 0, zero errors (two pre-existing![]()
warnings remain, unrelated to this fix). npm run build — clean production build, no type errors.
🤖 AI Assistance Declaration
Did you use an AI tool to write or assist with this code OR Pull Request?
** Which AI Model did you use?: Amazon Q Developer
** Which Platform/Tool?: Amazon Q (VS Code plugin)
** What exactly did the AI do?:
Identified the three offending files and patterns, proposed the derived-state-during-render fix, applied the code edits, ran ESLint after each change to verify, and iterated when the first attempt (using useRef during render) triggered a second lint rule (react-hooks/refs).
** What exactly did YOU do?:
Identified this as a repo-wide CI blocker affecting all contributors, provided the ESLint rule name and affected file list, reviewed each proposed fix for correctness, and verified the final lint output showed zero errors before committing.
** What is the advantage of using this AI approach here?:
The fix required understanding two separate ESLint rules (set-state-in-effect and refs) and their interaction. AI accelerated the iteration loop — first attempt used refs (wrong), second attempt used state-based previous-value tracking (correct) — without needing to manually re-run lint between each attempt.
✅ The "I Swear I Didn't Break Anything" Pledge
Summary by Sourcery
Resolve ESLint react-hooks/set-state-in-effect violations by moving state synchronization logic out of effects into render-time derived state patterns.
Bug Fixes:
Enhancements:
CI:
Summary by CodeRabbit
Bug Fixes
Refactor