Fix: High score calculation uses stale state (Issue #916) - #1436
Conversation
📝 WalkthroughWalkthroughVictory scoring now calculates the final bonus-adjusted score in one functional update. High-score persistence uses that final score instead of a captured stale value. The time bonus multiplier changes from 15 to 10. ChangesMemory Match scoring
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🤖 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/hooks/useMemoryMatch.js`:
- Around line 324-338: The setScore updater in useMemoryMatch must remain pure:
remove the nested setHighScore and localStorage.setItem side effects. Add a
post-commit useEffect that, when phase is "victory" and score exceeds highScore,
updates highScore and persists the new value to localStorage.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 024bc194-2d35-4bcc-8380-73c2ffe2a82b
📒 Files selected for processing (1)
frontend/src/hooks/useMemoryMatch.js
| // Apply the final bonuses to score and calculate high score accurately using functional update | ||
| setScore((prevScore) => { | ||
| let finalScore = prevScore; | ||
| if (timeElapsed < config.maxExpectedTime) finalScore += timeBonus; | ||
| if (isPerfect) finalScore += 500; | ||
|
|
||
| setHighScore((prevHigh) => { | ||
| if (finalScore > prevHigh) { | ||
| localStorage.setItem("memory_match_high_score", finalScore.toString()); | ||
| return finalScore; | ||
| } | ||
| return prevHigh; | ||
| }); | ||
|
|
||
| return finalScore; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
In React 18.3.1, can a useState functional updater safely call another state setter or localStorage.setItem? Find official React documentation on updater purity and Strict Mode updater invocation.
💡 Result:
No, a useState functional updater cannot safely call localStorage.setItem or perform other side effects [1][2]. Official React documentation explicitly states that updater functions must be pure [1][2]. React expects updater functions to perform only one task: calculate and return the next state based on the current state [1][2]. They must not change any objects or variables, nor should they perform any side effects like writing to localStorage, as these actions violate the requirement for purity [3][4][5]. This requirement is enforced during development by React's Strict Mode [6]. In Strict Mode, React intentionally calls your updater function twice [1][6]. If your updater function performs a side effect such as localStorage.setItem, that side effect will be executed twice [6][7]. While this may not always cause a crash, it is considered incorrect behavior that can lead to bugs, inconsistent application state, or performance issues, especially because React reserves the right to call these functions multiple times during the rendering phase [1][8][5]. To safely trigger side effects like updating localStorage when state changes, you should use the useEffect hook [3][4]. Alternatively, if you need to coordinate state updates with other logic, you can perform the side effect in an event handler or use a wrapped state setter pattern where the side effect is triggered alongside the state update, rather than inside the updater function itself [3][9]. Sources: [1] https://react.dev/reference/react/useState [3] https://react.dev/learn/keeping-components-pure [8] reactjs/react.dev#5982 [4] https://react.dev/reference/rules [6] https://react.dev/reference/react/StrictMode [7] https://legacy.reactjs.org/docs/strict-mode.html [5] https://stackoverflow.com/questions/71780110/is-it-okey-to-use-side-effects-in-the-usestate-hook-callback [2] https://react.dev/learn/queueing-a-series-of-state-updates [9] https://kyleshevlin.com/wrapped-state-setter-pattern/
Citations:
- 1: https://react.dev/reference/react/useState
- 2: https://react.dev/learn/queueing-a-series-of-state-updates
- 3: https://react.dev/learn/keeping-components-pure
- 4: https://react.dev/reference/rules
- 5: https://stackoverflow.com/questions/71780110/is-it-okey-to-use-side-effects-in-the-usestate-hook-callback
- 6: https://react.dev/reference/react/StrictMode
- 7: https://legacy.reactjs.org/docs/strict-mode.html
- 8: When does the callback inside the setter function of the useState hook execute? reactjs/react.dev#5982
- 9: https://kyleshevlin.com/wrapped-state-setter-pattern/
🏁 Script executed:
#!/bin/bash
set -eu
file="frontend/src/hooks/useMemoryMatch.js"
printf '%s\n' '--- target file outline ---'
ast-grep outline "$file" --lang javascript || true
printf '%s\n' '--- relevant state and effect references ---'
rg -n -C 4 'useState|useEffect|setScore|setHighScore|localStorage|phase|victory|finalScore' "$file"Repository: Canopus-Labs/PrepPilot
Length of output: 6517
🏁 Script executed:
#!/bin/bash
set -eu
file="frontend/src/hooks/useMemoryMatch.js"
printf '%s\n' '--- game initialization and reset ---'
sed -n '130,220p' "$file"
printf '%s\n' '--- victory implementation ---'
sed -n '288,342p' "$file"
printf '%s\n' '--- hook return and remaining effects ---'
sed -n '342,410p' "$file"
printf '%s\n' '--- project React version and tests ---'
rg -n '"react"|"react-dom"|StrictMode|useMemoryMatch|memory_match_high_score' \
package.json frontend/package.json frontend/src frontend/test* frontend/tests 2>/dev/null || trueRepository: Canopus-Labs/PrepPilot
Length of output: 20913
🌐 Web query:
site:react.dev/reference/react/useState updater function pure React may call updater twice Strict Mode side effects
💡 Result:
In React's Strict Mode, React intentionally calls your state updater functions twice during development [1][2]. This is a development-only behavior designed to help you identify accidental impurities in your code [1][3]. React uses the result of one of these calls and ignores the result of the other [1][4]. If your updater function is pure—meaning it does not produce side effects and consistently returns the same output for the same input—this double invocation will not affect your application's logic or behavior [1][5]. Because this mechanism is intended to enforce purity, if you notice issues when the function is called twice, it is a signal that your updater function may be performing side effects (such as modifying objects in place, performing network requests, or updating global variables), which should be moved to event handlers or useEffect hooks [1][2]. This behavior does not occur in production and does not affect event handlers, which do not need to be pure [1][6].
Citations:
- 1: https://react.dev/reference/react/useState
- 2: https://ru.react.dev/reference/react/useState
- 3: https://az.react.dev/reference/react/useState
- 4: https://ar.react.dev/reference/react/useState
- 5: https://de.react.dev/reference/react/useState
- 6: https://uk.react.dev/reference/react/useState
Keep the setScore updater pure.
Move setHighScore and localStorage.setItem to a post-commit useEffect. React Strict Mode can invoke functional updaters twice, which can persist a high score before the update commits. Synchronize the high score when phase === "victory" and score > highScore.
🤖 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/hooks/useMemoryMatch.js` around lines 324 - 338, The setScore
updater in useMemoryMatch must remain pure: remove the nested setHighScore and
localStorage.setItem side effects. Add a post-commit useEffect that, when phase
is "victory" and score exceeds highScore, updates highScore and persists the new
value to localStorage.
1 participant
Summary of What Has Been Done
In the Memory Match game component, the high score calculation was using the stale score state variable before React finished updating it asynchronously. This caused the game to sometimes save an incorrect, outdated high score upon completion.
Changes Made
Impact it Made
Closes #916
Summary
setScoreupdate.prevScore.setHighScore.